Prevent accidents with uninitialized nodes (#1139)

This comes from #1092 where I wrote bad code and I felt it should have been caught more clearly.
carbon-language · Mar 16, 2022 · 4a08b3a · 4a08b3a
1 parent 3457953
commit 4a08b3a
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 1 deletion.
diff --git a/toolchain/parser/parse_tree.cpp b/toolchain/parser/parse_tree.cpp
@@ -37,6 +37,7 @@ auto ParseTree::postorder() const -> llvm::iterator_range<PostorderIterator> {
 
 auto ParseTree::postorder(Node n) const
     -> llvm::iterator_range<PostorderIterator> {
+  CHECK(n.is_valid());
   // The postorder ends after this node, the root, and begins at the start of
   // its subtree.
   int end_index = n.index_ + 1;
@@ -47,6 +48,7 @@ auto ParseTree::postorder(Node n) const
 
 auto ParseTree::children(Node n) const
     -> llvm::iterator_range<SiblingIterator> {
+  CHECK(n.is_valid());
   int end_index = n.index_ - node_impls_[n.index_].subtree_size;
   return {SiblingIterator(*this, Node(n.index_ - 1)),
           SiblingIterator(*this, Node(end_index))};
@@ -59,18 +61,22 @@ auto ParseTree::roots() const -> llvm::iterator_range<SiblingIterator> {
 }
 
 auto ParseTree::node_has_error(Node n) const -> bool {
+  CHECK(n.is_valid());
   return node_impls_[n.index_].has_error;
 }
 
 auto ParseTree::node_kind(Node n) const -> ParseNodeKind {
+  CHECK(n.is_valid());
   return node_impls_[n.index_].kind;
 }
 
 auto ParseTree::node_token(Node n) const -> TokenizedBuffer::Token {
+  CHECK(n.is_valid());
   return node_impls_[n.index_].token;
 }
 
 auto ParseTree::GetNodeText(Node n) const -> llvm::StringRef {
+  CHECK(n.is_valid());
   return tokens_->GetTokenText(node_impls_[n.index_].token);
 }
 

diff --git a/toolchain/parser/parse_tree.h b/toolchain/parser/parse_tree.h
@@ -259,18 +259,25 @@ class ParseTree::Node {
   // Prints the node index.
   auto Print(llvm::raw_ostream& output) const -> void;
 
+  // Returns true if the node is valid; in other words, it was not default
+  // initialized.
+  auto is_valid() -> bool { return index_ != InvalidValue; }
+
  private:
   friend ParseTree;
   friend Parser;
   friend PostorderIterator;
   friend SiblingIterator;
 
+  // Value for uninitialized nodes.
+  static constexpr int InvalidValue = -1;
+
   // Constructs a node with a specific index into the parse tree's postorder
   // sequence of node implementations.
   explicit Node(int index) : index_(index) {}
 
   // The index of this node's implementation in the postorder sequence.
-  int32_t index_;
+  int32_t index_ = InvalidValue;
 };
 
 // A random-access iterator to the depth-first postorder sequence of parse nodes

diff --git a/toolchain/parser/parse_tree_test.cpp b/toolchain/parser/parse_tree_test.cpp
@@ -48,6 +48,17 @@ class ParseTreeTest : public ::testing::Test {
   DiagnosticConsumer& consumer = ConsoleDiagnosticConsumer();
 };
 
+TEST_F(ParseTreeTest, DefaultInvalid) {
+  ParseTree::Node node;
+  EXPECT_FALSE(node.is_valid());
+}
+
+TEST_F(ParseTreeTest, IsValid) {
+  TokenizedBuffer tokens = GetTokenizedBuffer("");
+  ParseTree tree = ParseTree::Parse(tokens, consumer);
+  EXPECT_TRUE((*tree.postorder().begin()).is_valid());
+}
+
 TEST_F(ParseTreeTest, Empty) {
   TokenizedBuffer tokens = GetTokenizedBuffer("");
   ParseTree tree = ParseTree::Parse(tokens, consumer);