From b76bc875c48b109c9a2aa764a9401745e87fbf3d Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 10 Feb 2023 12:00:50 -0800 Subject: [PATCH] Remove default constructor from IndexBase (#2598) There were some notes about default constructors being required in parse_tree.h, but they don't seem to be. Removing the default constructor forces the explicit `::Invalid` where there's no value immediately being assigned, which works fine for existing code. I think this actually reduces the chance of accidents more than the prior default-construct-as-invalid approach. --- toolchain/common/index_base.h | 2 +- toolchain/lexer/tokenized_buffer.h | 9 ++++++-- toolchain/parser/parse_tree.cpp | 4 ++-- toolchain/parser/parse_tree.h | 10 ++++----- toolchain/parser/parse_tree_test.cpp | 5 ----- toolchain/semantics/semantics_node.h | 22 ++++++++++--------- .../semantics_parse_tree_handler.cpp | 6 ++--- 7 files changed, 28 insertions(+), 30 deletions(-) diff --git a/toolchain/common/index_base.h b/toolchain/common/index_base.h index 6a623dc26879..1fd70b4f937e 100644 --- a/toolchain/common/index_base.h +++ b/toolchain/common/index_base.h @@ -24,7 +24,7 @@ class DataIterator; struct IndexBase { static constexpr int32_t InvalidIndex = -1; - constexpr IndexBase() : index(InvalidIndex) {} + IndexBase() = delete; constexpr explicit IndexBase(int index) : index(index) {} auto Print(llvm::raw_ostream& output) const -> void { diff --git a/toolchain/lexer/tokenized_buffer.h b/toolchain/lexer/tokenized_buffer.h index f5be0ce53f0b..039f06d5728f 100644 --- a/toolchain/lexer/tokenized_buffer.h +++ b/toolchain/lexer/tokenized_buffer.h @@ -81,6 +81,8 @@ class TokenizedBuffer { // All other APIs to query a `Identifier` are on the `TokenizedBuffer`. struct Identifier : public IndexBase { using IndexBase::IndexBase; + + static const Identifier Invalid; }; // Random-access iterator over tokens within the buffer. @@ -88,7 +90,7 @@ class TokenizedBuffer { : public llvm::iterator_facade_base< TokenIterator, std::random_access_iterator_tag, const Token, int> { public: - TokenIterator() = default; + TokenIterator() = delete; explicit TokenIterator(Token token) : token_(token) {} @@ -343,7 +345,7 @@ class TokenizedBuffer { sizeof(Token) <= sizeof(int32_t), "Unable to pack token and identifier index into the same space!"); - Identifier id; + Identifier id = Identifier::Invalid; int32_t literal_index; Token closing_token; Token opening_token; @@ -404,6 +406,9 @@ class TokenizedBuffer { bool has_errors_ = false; }; +constexpr TokenizedBuffer::Identifier TokenizedBuffer::Identifier::Invalid = + TokenizedBuffer::Identifier(TokenizedBuffer::Identifier::InvalidIndex); + // A diagnostic emitter that uses positions within a source buffer's text as // its source of location information. using LexerDiagnosticEmitter = DiagnosticEmitter; diff --git a/toolchain/parser/parse_tree.cpp b/toolchain/parser/parse_tree.cpp index 1fcd4475c4ac..4a2a8e4f595b 100644 --- a/toolchain/parser/parse_tree.cpp +++ b/toolchain/parser/parse_tree.cpp @@ -124,7 +124,7 @@ auto ParseTree::Print(llvm::raw_ostream& output) const -> void { } while (!node_stack.empty()) { - Node n; + Node n = Node::Invalid; int depth; std::tie(n, depth) = node_stack.pop_back_val(); for (Node sibling_n : children(n)) { @@ -161,7 +161,7 @@ auto ParseTree::Print(llvm::raw_ostream& output, bool preorder) const -> void { } while (!node_stack.empty()) { - Node n; + Node n = Node::Invalid; int depth; std::tie(n, depth) = node_stack.pop_back_val(); diff --git a/toolchain/parser/parse_tree.h b/toolchain/parser/parse_tree.h index de4e3a1360b4..34eb7ac7dc21 100644 --- a/toolchain/parser/parse_tree.h +++ b/toolchain/parser/parse_tree.h @@ -249,7 +249,8 @@ struct ParseTree::Node : public ComparableIndexBase { using ComparableIndexBase::ComparableIndexBase; }; -constexpr ParseTree::Node ParseTree::Node::Invalid; +constexpr ParseTree::Node ParseTree::Node::Invalid = + ParseTree::Node(ParseTree::Node::InvalidIndex); // A random-access iterator to the depth-first postorder sequence of parse nodes // in the parse tree. It produces `ParseTree::Node` objects which are opaque @@ -259,10 +260,7 @@ class ParseTree::PostorderIterator std::random_access_iterator_tag, Node, int, Node*, Node> { public: - // Default construction is only provided to satisfy iterator requirements. It - // produces an unusable iterator, and you must assign a valid iterator to it - // before performing any operations. - explicit PostorderIterator() = default; + PostorderIterator() = delete; auto operator==(const PostorderIterator& rhs) const -> bool { return node_ == rhs.node_; @@ -312,7 +310,7 @@ class ParseTree::SiblingIterator : public llvm::iterator_facade_base< SiblingIterator, std::forward_iterator_tag, Node, int, Node*, Node> { public: - explicit SiblingIterator() = default; + explicit SiblingIterator() = delete; auto operator==(const SiblingIterator& rhs) const -> bool { return node_ == rhs.node_; diff --git a/toolchain/parser/parse_tree_test.cpp b/toolchain/parser/parse_tree_test.cpp index 84d61ccaeab9..2955f9ec66ca 100644 --- a/toolchain/parser/parse_tree_test.cpp +++ b/toolchain/parser/parse_tree_test.cpp @@ -39,11 +39,6 @@ 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, /*vlog_stream=*/nullptr); diff --git a/toolchain/semantics/semantics_node.h b/toolchain/semantics/semantics_node.h index a8539a05c514..db86b734736b 100644 --- a/toolchain/semantics/semantics_node.h +++ b/toolchain/semantics/semantics_node.h @@ -33,7 +33,8 @@ struct SemanticsNodeId : public IndexBase { } }; -constexpr SemanticsNodeId SemanticsNodeId::Invalid = SemanticsNodeId(); +constexpr SemanticsNodeId SemanticsNodeId::Invalid = + SemanticsNodeId(SemanticsNodeId::InvalidIndex); // Uses the cross-reference node ID for a builtin. This relies on SemanticsIR // guarantees for builtin cross-reference placement. @@ -88,7 +89,7 @@ struct SemanticsNodeBlockId : public IndexBase { constexpr SemanticsNodeBlockId SemanticsNodeBlockId::Empty = SemanticsNodeBlockId(0); constexpr SemanticsNodeBlockId SemanticsNodeBlockId::Invalid = - SemanticsNodeBlockId(); + SemanticsNodeBlockId(SemanticsNodeBlockId::InvalidIndex); // Type-safe storage of strings. struct SemanticsStringId : public IndexBase { @@ -144,8 +145,8 @@ class SemanticsNode { SemanticsNodeId type) -> SemanticsNode { // Builtins won't have a ParseTree node associated, so we provide the // default invalid one. - return SemanticsNode(ParseTree::Node(), SemanticsNodeKind::Builtin, type, - builtin_kind.AsInt()); + return SemanticsNode(ParseTree::Node::Invalid, SemanticsNodeKind::Builtin, + type, builtin_kind.AsInt()); } auto GetAsBuiltin() const -> SemanticsBuiltinKind { CARBON_CHECK(kind_ == SemanticsNodeKind::Builtin); @@ -155,7 +156,7 @@ class SemanticsNode { static auto MakeCodeBlock(ParseTree::Node parse_node, SemanticsNodeBlockId node_block) -> SemanticsNode { return SemanticsNode(parse_node, SemanticsNodeKind::CodeBlock, - SemanticsNodeId(), node_block.index); + SemanticsNodeId::Invalid, node_block.index); } auto GetAsCodeBlock() const -> SemanticsNodeBlockId { CARBON_CHECK(kind_ == SemanticsNodeKind::CodeBlock); @@ -180,7 +181,7 @@ class SemanticsNode { SemanticsCallableId signature) -> SemanticsNode { return SemanticsNode(parse_node, SemanticsNodeKind::FunctionDeclaration, - SemanticsNodeId(), signature.index); + SemanticsNodeId::Invalid, signature.index); } auto GetAsFunctionDeclaration() const -> SemanticsCallableId { CARBON_CHECK(kind_ == SemanticsNodeKind::FunctionDeclaration); @@ -192,7 +193,8 @@ class SemanticsNode { SemanticsNodeBlockId node_block) -> SemanticsNode { return SemanticsNode(parse_node, SemanticsNodeKind::FunctionDefinition, - SemanticsNodeId(), decl.index, node_block.index); + SemanticsNodeId::Invalid, decl.index, + node_block.index); } auto GetAsFunctionDefinition() const -> std::pair { @@ -225,7 +227,7 @@ class SemanticsNode { // understand the type without checking, so it's not necessary but could be // specified if needed. return SemanticsNode(parse_node, SemanticsNodeKind::Return, - SemanticsNodeId()); + SemanticsNodeId::Invalid); } auto GetAsReturn() const -> NoArgs { CARBON_CHECK(kind_ == SemanticsNodeKind::Return); @@ -253,8 +255,8 @@ class SemanticsNode { } SemanticsNode() - : SemanticsNode(ParseTree::Node(), SemanticsNodeKind::Invalid, - SemanticsNodeId()) {} + : SemanticsNode(ParseTree::Node::Invalid, SemanticsNodeKind::Invalid, + SemanticsNodeId::Invalid) {} auto parse_node() const -> ParseTree::Node { return parse_node_; } auto kind() const -> SemanticsNodeKind { return kind_; } diff --git a/toolchain/semantics/semantics_parse_tree_handler.cpp b/toolchain/semantics/semantics_parse_tree_handler.cpp index d2796794c981..5f78cb5fccd4 100644 --- a/toolchain/semantics/semantics_parse_tree_handler.cpp +++ b/toolchain/semantics/semantics_parse_tree_handler.cpp @@ -324,10 +324,8 @@ auto SemanticsParseTreeHandler::HandleFunctionDefinitionStart( auto fn_node = node_stack_.PopForSoloParseNode(ParseNodeKind::FunctionIntroducer); - SemanticsCallable callable; - callable.param_ir_id = param_ir_id; - callable.param_refs_id = param_refs_id; - auto callable_id = semantics_->AddCallable(callable); + auto callable_id = semantics_->AddCallable( + {.param_ir_id = param_ir_id, .param_refs_id = param_refs_id}); auto decl_id = AddNode(SemanticsNode::MakeFunctionDeclaration(fn_node, callable_id)); // TODO: Propagate the type of the function.