Less leakage of container type in concrete syntax tree.

Reduces the leaky abstraction of handing out children() by
SyntaxTreeNode, but provide operations to work on them:
front(), back(), operator[].

This change does not get rid of handing out children() entirely yet,
as they are used in range loops, this will be a separate change.

Reducing the abstraction leak will make it easier to change that
implementation later (e.g. for issue #1523)

common/analysis/matcher/matcher_test.cc

@@ -156,8 +156,8 @@ TEST(MatcherTest, BindMatcherNested) {
 static std::vector<const Symbol *> GetFirstChild(const Symbol &symbol) {
   if (symbol.Kind() == SymbolKind::kNode) {
     const auto &node = down_cast<const SyntaxTreeNode &>(symbol);
-    if (node.children().empty()) return {};
-    return {node.children()[0].get()};
+    if (node.empty()) return {};
+    return {node[0].get()};
   }
 
   return {};

common/text/concrete_syntax_tree.cc

@@ -41,13 +41,13 @@ bool SyntaxTreeNode::equals(const Symbol *symbol,
 bool SyntaxTreeNode::equals(const SyntaxTreeNode *node,
                             const TokenComparator &compare_tokens) const {
   if (Tag().tag != node->Tag().tag) return false;
-  if (children_.size() != node->children().size()) {
+  if (children_.size() != node->size()) {
     return false;
   }
-  int size = children_.size();
-  for (int i = 0; i < size; i++) {
-    if (!EqualTrees(children_[i].get(), node->children()[i].get(),
-                    compare_tokens)) {
+  auto this_it = children_.begin();
+  auto other_it = node->children().begin();
+  for (/**/; this_it != children_.end(); ++this_it, ++other_it) {
+    if (!EqualTrees(this_it->get(), other_it->get(), compare_tokens)) {
       return false;
     }
   }
@@ -89,10 +89,10 @@ void SetChild_(const SymbolPtr &parent, int child_index, SymbolPtr new_child) {
   CHECK_EQ(ABSL_DIE_IF_NULL(parent)->Kind(), SymbolKind::kNode);
 
   auto *parent_node = down_cast<SyntaxTreeNode *>(parent.get());
-  CHECK_LT(child_index, static_cast<int>(parent_node->children().size()));
-  CHECK(parent_node->children()[child_index] == nullptr);
+  CHECK_LT(child_index, static_cast<int>(parent_node->size()));
+  CHECK((*parent_node)[child_index] == nullptr);
 
-  parent_node->mutable_children()[child_index] = std::move(new_child);
+  (*parent_node)[child_index] = std::move(new_child);
 }
 
 }  // namespace verible

common/text/concrete_syntax_tree.h

@@ -47,6 +47,7 @@
 #include "common/text/tree_compare.h"
 #include "common/text/visitors.h"
 #include "common/util/casts.h"
+#include "common/util/iterator_range.h"
 #include "common/util/logging.h"
 
 namespace verible {
@@ -70,29 +71,32 @@ struct ForwardChildren {
 // used by various language front-ends.
 class SyntaxTreeNode final : public Symbol {
  public:
-  explicit SyntaxTreeNode(const int tag = kUntagged) : tag_(tag) {}
+  // This container needs to provide a random access [] operator and
+  // rbegin(), rend() iterators.
+  using ChildContainer = std::vector<SymbolPtr>;
 
-  const std::vector<SymbolPtr> &children() const { return children_; }
-  std::vector<SymbolPtr> &mutable_children() { return children_; }
+  explicit SyntaxTreeNode(const int tag = kUntagged) : tag_(tag) {}
 
   // Transfer ownership of argument to this object.
   // Call MakeNode or ExtendNode instead of calling this directly.
-  void AppendChild(SymbolPtr child) { children_.push_back(std::move(child)); }
+  void AppendChild(SymbolPtr child) {
+    children_.emplace_back(std::move(child));
+  }
 
   // Transfer ownership of argument's children to this object.
   // Call MakeNode or ExtendNode instead of calling this directly.
   // If node is actually a leaf, just append the leaf.
   void AppendChild(ForwardChildren forwarded_children) {
     if (forwarded_children.node == nullptr) return;
     if (forwarded_children.node->Kind() != SymbolKind::kNode) {
-      children_.push_back(std::move(forwarded_children.node));
+      children_.emplace_back(std::move(forwarded_children.node));
       return;
     }
     auto *node = down_cast<SyntaxTreeNode *>(forwarded_children.node.get());
     const auto new_size = children_.size() + node->children_.size();
     children_.reserve(new_size);
     for (auto &child : node->children_) {
-      children_.push_back(std::move(child));
+      children_.emplace_back(std::move(child));
     }
     // Remove all the vacated children slots left in the parent.
     node->children_.clear();
@@ -115,6 +119,19 @@ class SyntaxTreeNode final : public Symbol {
   // Children accessor (const).
   const SymbolPtr &operator[](size_t i) const;
 
+  const SymbolPtr &front() const { return children_.front(); }
+  SymbolPtr &front() { return children_.front(); }
+
+  const SymbolPtr &back() const { return children_.back(); }
+  SymbolPtr &back() { return children_.back(); }
+
+  size_t size() const { return children_.size(); }
+  bool empty() const { return children_.empty(); }
+
+  // TODO(hzeller): return ranges for these. Only used in range-loops.
+  const ChildContainer &children() const { return children_; }
+  ChildContainer &mutable_children() { return children_; }
+
   // Compares this node to an arbitrary symbol using the compare_tokens
   // function.
   bool equals(const Symbol *symbol,
@@ -178,7 +195,7 @@ class SyntaxTreeNode final : public Symbol {
   int tag_;
 
   // Sequence of pointers to subtrees and nodes.
-  std::vector<SymbolPtr> children_;
+  ChildContainer children_;
 };
 
 // The following functions are intended for use in semantic action blocks

common/text/concrete_syntax_tree_test.cc

@@ -68,37 +68,37 @@ TEST(SyntaxTreeNodeMatchesTagAnyOf, Matches) {
 TEST(SyntaxTreeNodeAppend, AppendVoid) {
   SyntaxTreeNode node;
   node.Append();
-  EXPECT_THAT(node.children(), IsEmpty());
+  EXPECT_THAT(node, IsEmpty());
 }
 
 // Test that std::move is automated.
 TEST(SyntaxTreeNodeAppend, AppendChildReference) {
   SyntaxTreeNode node;
   SymbolPtr child;
   node.Append(child);
-  EXPECT_THAT(node.children(), SizeIs(1));
+  EXPECT_THAT(node, SizeIs(1));
 }
 
 // Test that redundant move is accepted.
 TEST(SyntaxTreeNodeAppend, AppendChildMoved) {
   SyntaxTreeNode node;
   SymbolPtr child;
   node.Append(std::move(child));
-  EXPECT_THAT(node.children(), SizeIs(1));
+  EXPECT_THAT(node, SizeIs(1));
 }
 
 // Test that temporary value is properly forwarded.
 TEST(SyntaxTreeNodeAppend, AppendChildTemporary) {
   SyntaxTreeNode node;
   node.Append(SymbolPtr());
-  EXPECT_THAT(node.children(), SizeIs(1));
+  EXPECT_THAT(node, SizeIs(1));
 }
 
 // Test that nullptrs can be appended.
 TEST(SyntaxTreeNodeAppend, AppendChildNullPtr) {
   SyntaxTreeNode node;
   node.Append(nullptr);
-  EXPECT_THAT(node.children(), SizeIs(1));
+  EXPECT_THAT(node, SizeIs(1));
 }
 
 // Test that ownership is transferred to sink functions.
@@ -128,14 +128,14 @@ TEST(MakeNodeTest, TaggedEmptyConstructor) {
   const int tag = 10;
   auto node = MakeTaggedNode(tag);
   ASSERT_THAT(node, NotNull());
-  EXPECT_THAT(CheckTree(node)->children(), IsEmpty());
+  EXPECT_THAT(*CheckTree(node), IsEmpty());
 }
 
 // Test construction of tagged node.
 TEST(MakeNodeTest, ImmediateTaggedEmptyConstructor) {
   auto node = MakeTaggedNode(20);
   ASSERT_THAT(node, NotNull());
-  EXPECT_THAT(CheckTree(node)->children(), IsEmpty());
+  EXPECT_THAT(*CheckTree(node), IsEmpty());
 }
 
 // Test construction of untagged node with one child.
@@ -145,14 +145,14 @@ TEST(MakeNodeTest, SingleChild) {
   auto parent = MakeNode(child);
   EXPECT_THAT(parent, NotNull());
   EXPECT_THAT(child, IsNull());
-  EXPECT_THAT(CheckTree(parent)->children(), SizeIs(1));
+  EXPECT_THAT(*CheckTree(parent), SizeIs(1));
 }
 
 // Test construction of untagged node with one (temporary) child.
 TEST(MakeNodeTest, SingleChildTemporary) {
   auto parent = MakeNode(MakeNode());
   EXPECT_THAT(parent, NotNull());
-  EXPECT_THAT(CheckTree(parent)->children(), SizeIs(1));
+  EXPECT_THAT(*CheckTree(parent), SizeIs(1));
 }
 
 // Test construction of untagged node with multiple children.
@@ -168,7 +168,7 @@ TEST(MakeNodeTest, MultiChild) {
   EXPECT_THAT(child1, IsNull());
   EXPECT_THAT(child2, IsNull());
   EXPECT_THAT(child3, IsNull());
-  EXPECT_THAT(CheckTree(parent)->children(), SizeIs(3));
+  EXPECT_THAT(*CheckTree(parent), SizeIs(3));
 }
 
 // Test ExtendNode with nothing to extend (base case).
@@ -178,7 +178,7 @@ TEST(ExtendNodeTest, ExtendNone) {
   auto seq2 = ExtendNode(seq);
   EXPECT_THAT(seq2, NotNull());
   EXPECT_THAT(seq, IsNull());
-  EXPECT_THAT(CheckTree(seq2)->children(), IsEmpty());
+  EXPECT_THAT(*CheckTree(seq2), IsEmpty());
 }
 
 // Test extending node with one child.
@@ -191,7 +191,7 @@ TEST(ExtendNodeTest, ExtendOne) {
   EXPECT_THAT(seq2, NotNull());
   EXPECT_THAT(seq, IsNull());
   EXPECT_THAT(item, IsNull());
-  EXPECT_THAT(CheckTree(seq2)->children(), SizeIs(1));
+  EXPECT_THAT(*CheckTree(seq2), SizeIs(1));
 }
 
 // Test extending node with multiple children.
@@ -210,7 +210,7 @@ TEST(ExtendNodeTest, ExtendMulti) {
   EXPECT_THAT(item1, IsNull());
   EXPECT_THAT(item2, IsNull());
   EXPECT_THAT(item3, IsNull());
-  EXPECT_THAT(CheckTree(seq2)->children(), SizeIs(3));
+  EXPECT_THAT(*CheckTree(seq2), SizeIs(3));
 }
 
 // Test extending node with multiple (temporary) children.
@@ -220,37 +220,37 @@ TEST(ExtendNodeTest, ExtendMultiTemporary) {
   auto seq2 = ExtendNode(seq, MakeNode(), MakeNode());
   ASSERT_THAT(seq2, NotNull());
   EXPECT_THAT(seq, IsNull());
-  EXPECT_THAT(CheckTree(seq2)->children(), SizeIs(2));
+  EXPECT_THAT(*CheckTree(seq2), SizeIs(2));
 }
 
 // Test extending node with temporary parent.
 TEST(ExtendNodeTest, ExtendTemporaryNodeNoChildren) {
   auto seq = ExtendNode(MakeNode());
   ASSERT_THAT(seq, NotNull());
-  EXPECT_THAT(CheckTree(seq)->children(), IsEmpty());
+  EXPECT_THAT(*CheckTree(seq), IsEmpty());
 }
 
 // Test extending node with temporary parent with temporary children.
 TEST(ExtendNodeTest, ExtendTemporaryNode) {
   auto seq = ExtendNode(MakeNode(), MakeNode(), MakeNode());
   ASSERT_THAT(seq, NotNull());
-  EXPECT_THAT(CheckTree(seq)->children(), SizeIs(2));
+  EXPECT_THAT(*CheckTree(seq), SizeIs(2));
 }
 
 // Test forwarding empty set of children to new node.
 TEST(SyntaxTreeNodeAppend, AdoptChildrenNone) {
   auto seq = MakeNode();
   auto parent = MakeNode(ForwardChildren(seq));
   EXPECT_THAT(seq, IsNull());
-  EXPECT_THAT(CheckTree(parent)->children(), IsEmpty());
+  EXPECT_THAT(*CheckTree(parent), IsEmpty());
 }
 
 // Test forwarding empty set of children to new node.
 TEST(SyntaxTreeNodeAppend, AdoptLeaf) {
   SymbolPtr leaf(new SyntaxTreeLeaf(0, "abc"));
   auto parent = MakeNode(ForwardChildren(leaf));
   EXPECT_THAT(leaf, IsNull());
-  EXPECT_THAT(CheckTree(parent)->children(), SizeIs(1));
+  EXPECT_THAT(*CheckTree(parent), SizeIs(1));
 }
 
 // Test forwarding set of children to new node, transferring ownership.
@@ -259,7 +259,7 @@ TEST(SyntaxTreeNodeAppend, AdoptChildren) {
   auto parent = MakeNode(ForwardChildren(seq));
   EXPECT_THAT(seq, IsNull());
   auto parentnode = CheckTree(parent);
-  EXPECT_THAT(parentnode->children(), SizeIs(3));
+  EXPECT_THAT(*parentnode, SizeIs(3));
   for (const auto &child : parentnode->children()) {
     EXPECT_THAT(child, NotNull());
   }
@@ -271,7 +271,7 @@ TEST(SyntaxTreeNodeAppend, AdoptNullChildren) {
   auto parent = MakeNode(ForwardChildren(seq));
   EXPECT_THAT(seq, IsNull());
   auto parentnode = CheckTree(parent);
-  EXPECT_THAT(parentnode->children(), SizeIs(2));
+  EXPECT_THAT(*parentnode, SizeIs(2));
   for (const auto &child : parentnode->children()) {
     EXPECT_THAT(child, IsNull());
   }
@@ -303,7 +303,7 @@ TEST(SyntaxTreeNodeAppend, AdoptChildrenMixed) {
   EXPECT_THAT(seq, IsNull());
   auto parentnode = CheckTree(parent);
   ASSERT_THAT(parentnode, NotNull());
-  EXPECT_THAT(parentnode->children(), SizeIs(5));
+  EXPECT_THAT(*parentnode, SizeIs(5));
   for (const auto &child : parentnode->children()) {
     EXPECT_THAT(child, NotNull());
   }
@@ -318,7 +318,7 @@ TEST(SyntaxTreeNodeAppend, AdoptChildrenMultiple) {
   EXPECT_THAT(seq2, IsNull());
   auto parentnode = CheckTree(parent);
   ASSERT_THAT(parentnode, NotNull());
-  EXPECT_THAT(parentnode->children(), SizeIs(7));
+  EXPECT_THAT(*parentnode, SizeIs(7));
   for (const auto &child : parentnode->children()) {
     EXPECT_THAT(child, NotNull());
   }
@@ -330,7 +330,7 @@ TEST(ExtendNodeTest, AdoptChildren) {
   auto parent = MakeNode(ForwardChildren(seq));
   EXPECT_THAT(seq, IsNull());
   auto parentnode = CheckTree(parent);
-  EXPECT_THAT(parentnode->children(), SizeIs(2));
+  EXPECT_THAT(*parentnode, SizeIs(2));
   for (const auto &child : parentnode->children()) {
     EXPECT_THAT(child, NotNull());
   }
@@ -342,7 +342,7 @@ TEST(ExtendNodeTest, AdoptChildrenMixed) {
   auto parent = ExtendNode(MakeNode(), ForwardChildren(seq), MakeNode());
   EXPECT_THAT(seq, IsNull());
   auto parentnode = CheckTree(parent);
-  EXPECT_THAT(parentnode->children(), SizeIs(4));
+  EXPECT_THAT(*parentnode, SizeIs(4));
   for (const auto &child : parentnode->children()) {
     EXPECT_THAT(child, NotNull());
   }
@@ -357,7 +357,7 @@ TEST(ExtendNodeTest, SetChild0Size1) {
   SetChild(node1, 0, node2);
   EXPECT_THAT(node1, NotNull());
   EXPECT_THAT(node2, IsNull());
-  EXPECT_THAT(CheckTree(node1)->children(), SizeIs(1));
+  EXPECT_THAT(*CheckTree(node1), SizeIs(1));
   EXPECT_TRUE(EqualTreesByEnum(expected.get(), node1.get()));
 }
 
@@ -370,7 +370,7 @@ TEST(ExtendNodeTest, SetChild1Size2) {
   SetChild(node1, 1, node2);
   EXPECT_THAT(node1, NotNull());
   EXPECT_THAT(node2, IsNull());
-  EXPECT_THAT(CheckTree(node1)->children(), SizeIs(2));
+  EXPECT_THAT(*CheckTree(node1), SizeIs(2));
   EXPECT_TRUE(EqualTreesByEnum(expected.get(), node1.get()));
 }
 

common/text/text_structure_test.cc

@@ -484,9 +484,8 @@ TEST_F(TextStructureViewPublicTest, ExpandSubtreesOneLeaf) {
   std::string subtext(tokens_[0].text().data(), tokens_[0].text().length());
   std::unique_ptr<TextStructure> subanalysis(new TextStructure(subtext));
   FakeParseToken(&subanalysis->MutableData(), divide, new_node_tag);
-  auto &replacement_node = down_cast<SyntaxTreeNode *>(syntax_tree_.get())
-                               ->mutable_children()
-                               .front();
+  auto &replacement_node =
+      down_cast<SyntaxTreeNode *>(syntax_tree_.get())->front();
   TextStructureView::DeferredExpansion expansion{&replacement_node,
                                                  std::move(subanalysis)};
   // Expect tree must be built using substrings of contents_.
@@ -518,9 +517,8 @@ TEST_F(TextStructureViewPublicTest, ExpandSubtreesMultipleLeaves) {
     std::string subtext(tokens_[0].text().data(), tokens_[0].text().length());
     std::unique_ptr<TextStructure> subanalysis(new TextStructure(subtext));
     FakeParseToken(&subanalysis->MutableData(), divide1, new_node_tag1);
-    auto &replacement_node = down_cast<SyntaxTreeNode *>(syntax_tree_.get())
-                                 ->mutable_children()
-                                 .front();
+    auto &replacement_node =
+        down_cast<SyntaxTreeNode *>(syntax_tree_.get())->front();
     TextStructureView::DeferredExpansion expansion{&replacement_node,
                                                    std::move(subanalysis)};
     expansion_map[tokens_[0].left(contents_)] = std::move(expansion);
@@ -530,9 +528,8 @@ TEST_F(TextStructureViewPublicTest, ExpandSubtreesMultipleLeaves) {
     std::string subtext(tokens_[3].text().data(), tokens_[3].text().length());
     std::unique_ptr<TextStructure> subanalysis(new TextStructure(subtext));
     FakeParseToken(&subanalysis->MutableData(), divide2, new_node_tag2);
-    auto &replacement_node = down_cast<SyntaxTreeNode *>(syntax_tree_.get())
-                                 ->mutable_children()
-                                 .back();
+    auto &replacement_node =
+        down_cast<SyntaxTreeNode *>(syntax_tree_.get())->back();
     TextStructureView::DeferredExpansion expansion{&replacement_node,
                                                    std::move(subanalysis)};
     expansion_map[offset2] = std::move(expansion);