From 29b6399e4f8c0542bc11c1a59b477b30e46fd7e3 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 25 Aug 2023 17:17:25 -0700 Subject: [PATCH] Modify EnumBase to better support the namespacing of toolchain (#3156) The different approach to Names avoids the issues with trying to define a static member (or also member function) of the templated instance of Carbon::Internal::EnumBase from a non-enclosing namespace such as Carbon::SemIR. Note, I'm trying to do this from the cpp file. An alternative might be to do `inline constexpr llvm::StringLiteral Names[]` in the .h file, but I think concerns had been raised about that needing deduplication. --- .clang-format | 3 +- common/enum_base.h | 64 ++++++------------- explorer/ast/ast_rtti.cpp | 1 - explorer/ast/ast_rtti.h | 7 +- .../semantics/semantics_builtin_kind.cpp | 8 +-- toolchain/semantics/semantics_builtin_kind.h | 22 +++---- toolchain/semantics/semantics_node.cpp | 14 ++-- toolchain/semantics/semantics_node.h | 13 ++-- toolchain/semantics/semantics_node_kind.cpp | 20 +++--- toolchain/semantics/semantics_node_kind.h | 23 ++----- 10 files changed, 65 insertions(+), 110 deletions(-) diff --git a/.clang-format b/.clang-format index a97d54235429..02cfd9df7a51 100644 --- a/.clang-format +++ b/.clang-format @@ -13,5 +13,6 @@ FixNamespaceComments: 'true' PointerAlignment: Left # We abuse control macros for formatting other kinds of macros. SpaceBeforeParens: ControlStatementsExceptControlMacros -IfMacros: ['CARBON_DEFINE_RAW_ENUM_CLASS'] +IfMacros: + ['CARBON_DEFINE_RAW_ENUM_CLASS', 'CARBON_DEFINE_RAW_ENUM_CLASS_NO_NAMES'] StatementMacros: ['ABSTRACT'] diff --git a/common/enum_base.h b/common/enum_base.h index d36d9eadc440..c447fbc82172 100644 --- a/common/enum_base.h +++ b/common/enum_base.h @@ -51,7 +51,7 @@ namespace Carbon::Internal { // #include ".../my_kind.def" // }; // ``` -template +template class EnumBase { public: // An alias for the raw enum type. This is an implementation detail and @@ -78,12 +78,10 @@ class EnumBase { // This method will be automatically defined using the static `names` string // table in the base class, which is in turn will be populated for each // derived type using the macro helpers in this file. - [[nodiscard]] auto name() const -> llvm::StringRef; + [[nodiscard]] auto name() const -> llvm::StringRef { return Names[AsInt()]; } // Prints this value using its name. - void Print(llvm::raw_ostream& out) const { - out << static_cast(this)->name(); - } + auto Print(llvm::raw_ostream& out) const -> void { out << name(); } protected: // The default constructor is explicitly defaulted (and constexpr) as a @@ -112,22 +110,27 @@ class EnumBase { } private: - static llvm::StringLiteral names[]; - RawEnumType value_; }; } // namespace Carbon::Internal +// For use when multiple enums use the same list of names. +#define CARBON_DEFINE_RAW_ENUM_CLASS_NO_NAMES(EnumClassName, UnderlyingType) \ + namespace Internal { \ + /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ + enum class EnumClassName##RawEnum : UnderlyingType; \ + } \ + enum class Internal::EnumClassName##RawEnum : UnderlyingType + // Use this before defining a class that derives from `EnumBase` to begin the // definition of the raw `enum class`. It should be followed by the body of that // raw enum class. #define CARBON_DEFINE_RAW_ENUM_CLASS(EnumClassName, UnderlyingType) \ namespace Internal { \ - /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ - enum class EnumClassName##RawEnum : UnderlyingType; \ + extern const llvm::StringLiteral EnumClassName##Names[]; \ } \ - enum class ::Carbon::Internal::EnumClassName##RawEnum : UnderlyingType + CARBON_DEFINE_RAW_ENUM_CLASS_NO_NAMES(EnumClassName, UnderlyingType) // In CARBON_DEFINE_RAW_ENUM_CLASS block, use this to generate each enumerator. #define CARBON_RAW_ENUM_ENUMERATOR(Name) Name, @@ -136,12 +139,14 @@ class EnumBase { // class. It both computes the name of the raw enum and ensures all the // namespaces are correct. #define CARBON_ENUM_BASE(EnumClassName) \ - CARBON_ENUM_BASE_CRTP(EnumClassName, EnumClassName) + CARBON_ENUM_BASE_CRTP(EnumClassName, EnumClassName, EnumClassName) // This variant handles the case where the external name for the Carbon enum is // not the same as the name by which we refer to it from this context. -#define CARBON_ENUM_BASE_CRTP(EnumClassName, LocalTypeNameForEnumClass) \ +#define CARBON_ENUM_BASE_CRTP(EnumClassName, LocalTypeNameForEnumClass, \ + EnumClassNameForNames) \ ::Carbon::Internal::EnumBase + Internal::EnumClassName##RawEnum, \ + Internal::EnumClassNameForNames##Names> // Use this within the Carbon enum class body to generate named constant // declarations for each value. @@ -163,43 +168,14 @@ class EnumBase { static constexpr const typename Base::EnumType& Name = \ Base::Create(Base::RawEnumType::Name); -// Use this to define a custom `name()` function for an enum-like class. Usage: -// -// CARBON_ENUM_NAME_FUNCTION(MyEnum) { -// // Return a StringRef based on the value of *this. -// } -// -// You should usually use CARBON_DEFINE_ENUM_CLASS_NAMES instead. -#define CARBON_ENUM_NAME_FUNCTION(EnumClassName) \ - template <> \ - auto \ - Internal::EnumBase::name() \ - const->llvm::StringRef - // Use this in the `.cpp` file for an enum class to start the definition of the // constant names array for each enumerator. It is followed by the desired // constant initializer. // // `clang-format` has a bug with spacing around `->` returns in macros. See // https://bugs.llvm.org/show_bug.cgi?id=48320 for details. -#define CARBON_DEFINE_ENUM_CLASS_NAMES(EnumClassName) \ - /* First declare an explicit specialization of the names array so we can \ - * reference it from an explicit function specialization. */ \ - template <> \ - llvm::StringLiteral Internal::EnumBase< \ - EnumClassName, Internal::EnumClassName##RawEnum>::names[]; \ - \ - /* Now define an explicit function specialization for the `name` method, as \ - * it can now reference our specialized array. */ \ - CARBON_ENUM_NAME_FUNCTION(EnumClassName) { \ - return names[static_cast(value_)]; \ - } \ - \ - /* Finally, open up the definition of our specialized array for the user to \ - * populate using the x-macro include. */ \ - template <> \ - llvm::StringLiteral Internal::EnumBase< \ - EnumClassName, Internal::EnumClassName##RawEnum>::names[] +#define CARBON_DEFINE_ENUM_CLASS_NAMES(EnumClassName) \ + constexpr llvm::StringLiteral Internal::EnumClassName##Names[] // Use this within the names array initializer to generate a string for each // name. diff --git a/explorer/ast/ast_rtti.cpp b/explorer/ast/ast_rtti.cpp index 6c718ac9b151..a8a0cac3da35 100644 --- a/explorer/ast/ast_rtti.cpp +++ b/explorer/ast/ast_rtti.cpp @@ -15,6 +15,5 @@ CARBON_DEFINE_ENUM_CLASS_NAMES(AstRttiNodeKind) = { CARBON_ENUM_NAME_FUNCTION(C##Kind) { \ return AstRttiNodeKind(static_cast(*this)).name(); \ } -CARBON_AST_FOR_EACH_ABSTRACT_CLASS(DEFINE_NAME_FUNCTION) } // namespace Carbon diff --git a/explorer/ast/ast_rtti.h b/explorer/ast/ast_rtti.h index eb458d73a28e..3b6dc9c42510 100644 --- a/explorer/ast/ast_rtti.h +++ b/explorer/ast/ast_rtti.h @@ -46,13 +46,14 @@ CARBON_AST_FOR_EACH_FINAL_CLASS(CONSTANT_DEFINITION) // Define Kind enumerations for all base classes. #define DEFINE_KIND_ENUM(C) \ - CARBON_DEFINE_RAW_ENUM_CLASS(C##Kind, int) { \ + CARBON_DEFINE_RAW_ENUM_CLASS_NO_NAMES(C##Kind, int) { \ CARBON_AST_FOR_EACH_FINAL_CLASS_BELOW(C, DEFINE_ENUMERATOR) \ }; \ template \ - class C##KindTemplate : public CARBON_ENUM_BASE_CRTP(C##Kind, Derived) { \ + class C##KindTemplate \ + : public CARBON_ENUM_BASE_CRTP(C##Kind, Derived, AstRttiNodeKind) { \ private: \ - using Base = CARBON_ENUM_BASE_CRTP(C##Kind, Derived); \ + using Base = CARBON_ENUM_BASE_CRTP(C##Kind, Derived, AstRttiNodeKind); \ friend class AstRttiNodeKind; \ \ public: \ diff --git a/toolchain/semantics/semantics_builtin_kind.cpp b/toolchain/semantics/semantics_builtin_kind.cpp index 8ec0f80f03d8..d0a433100920 100644 --- a/toolchain/semantics/semantics_builtin_kind.cpp +++ b/toolchain/semantics/semantics_builtin_kind.cpp @@ -4,15 +4,15 @@ #include "toolchain/semantics/semantics_builtin_kind.h" -namespace Carbon { +namespace Carbon::SemIR { -CARBON_DEFINE_ENUM_CLASS_NAMES(SemanticsBuiltinKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(BuiltinKind) = { #define CARBON_SEMANTICS_BUILTIN_KIND_NAME(Name) \ CARBON_ENUM_CLASS_NAME_STRING(Name) #include "toolchain/semantics/semantics_builtin_kind.def" }; -auto SemanticsBuiltinKind::label() -> llvm::StringRef { +auto BuiltinKind::label() -> llvm::StringRef { static constexpr llvm::StringLiteral Labels[] = { #define CARBON_SEMANTICS_BUILTIN_KIND(Name, Label) Label, #include "toolchain/semantics/semantics_builtin_kind.def" @@ -20,4 +20,4 @@ auto SemanticsBuiltinKind::label() -> llvm::StringRef { return Labels[AsInt()]; } -} // namespace Carbon +} // namespace Carbon::SemIR diff --git a/toolchain/semantics/semantics_builtin_kind.h b/toolchain/semantics/semantics_builtin_kind.h index 1a9ef965974b..a2b3a577a521 100644 --- a/toolchain/semantics/semantics_builtin_kind.h +++ b/toolchain/semantics/semantics_builtin_kind.h @@ -9,15 +9,15 @@ #include "common/enum_base.h" -namespace Carbon { +namespace Carbon::SemIR { -CARBON_DEFINE_RAW_ENUM_CLASS(SemanticsBuiltinKind, uint8_t) { +CARBON_DEFINE_RAW_ENUM_CLASS(BuiltinKind, uint8_t) { #define CARBON_SEMANTICS_BUILTIN_KIND_NAME(Name) \ CARBON_RAW_ENUM_ENUMERATOR(Name) #include "toolchain/semantics/semantics_builtin_kind.def" }; -class SemanticsBuiltinKind : public CARBON_ENUM_BASE(SemanticsBuiltinKind) { +class BuiltinKind : public CARBON_ENUM_BASE(BuiltinKind) { public: #define CARBON_SEMANTICS_BUILTIN_KIND_NAME(Name) \ CARBON_ENUM_CONSTANT_DECLARATION(Name) @@ -37,26 +37,20 @@ class SemanticsBuiltinKind : public CARBON_ENUM_BASE(SemanticsBuiltinKind) { }; #define CARBON_SEMANTICS_BUILTIN_KIND_NAME(Name) \ - CARBON_ENUM_CONSTANT_DEFINITION(SemanticsBuiltinKind, Name) + CARBON_ENUM_CONSTANT_DEFINITION(BuiltinKind, Name) #include "toolchain/semantics/semantics_builtin_kind.def" -constexpr uint8_t SemanticsBuiltinKind::ValidCount = Invalid.AsInt(); +constexpr uint8_t BuiltinKind::ValidCount = Invalid.AsInt(); static_assert( - SemanticsBuiltinKind::ValidCount != 0, + BuiltinKind::ValidCount != 0, "The above `constexpr` definition of `ValidCount` makes it available in " "a `constexpr` context despite being declared as merely `const`. We use it " "in a static assert here to ensure that."); // We expect the builtin kind to fit compactly into 8 bits. -static_assert(sizeof(SemanticsBuiltinKind) == 1, - "Kind objects include padding!"); +static_assert(sizeof(BuiltinKind) == 1, "Kind objects include padding!"); -// TODO: Refactor EnumBase to remove the need for this alias. -namespace SemIR { -using BuiltinKind = SemanticsBuiltinKind; -} // namespace SemIR - -} // namespace Carbon +} // namespace Carbon::SemIR #endif // CARBON_TOOLCHAIN_SEMANTICS_SEMANTICS_BUILTIN_KIND_H_ diff --git a/toolchain/semantics/semantics_node.cpp b/toolchain/semantics/semantics_node.cpp index 4ceccceb709e..118871f96fa2 100644 --- a/toolchain/semantics/semantics_node.cpp +++ b/toolchain/semantics/semantics_node.cpp @@ -20,23 +20,21 @@ static auto PrintArgs(llvm::raw_ostream& out, std::pair args) -> void { out << ", arg1: " << args.second; } -auto operator<<(llvm::raw_ostream& out, const Node& node) - -> llvm::raw_ostream& { - out << "{kind: " << node.kind_; +auto Node::Print(llvm::raw_ostream& out) const -> void { + out << "{kind: " << kind_; // clang warns on unhandled enum values; clang-tidy is incorrect here. // NOLINTNEXTLINE(bugprone-switch-missing-default-case) - switch (node.kind_) { + switch (kind_) { #define CARBON_SEMANTICS_NODE_KIND(Name) \ case NodeKind::Name: \ - PrintArgs(out, node.GetAs##Name()); \ + PrintArgs(out, GetAs##Name()); \ break; #include "toolchain/semantics/semantics_node_kind.def" } - if (node.type_id_.is_valid()) { - out << ", type: " << node.type_id_; + if (type_id_.is_valid()) { + out << ", type: " << type_id_; } out << "}"; - return out; } } // namespace Carbon::SemIR diff --git a/toolchain/semantics/semantics_node.h b/toolchain/semantics/semantics_node.h index 8851256f1abb..d232e2877b2a 100644 --- a/toolchain/semantics/semantics_node.h +++ b/toolchain/semantics/semantics_node.h @@ -233,13 +233,10 @@ class Node { // Factory base classes are private, then used for public classes. This class // has two public and two private sections to prevent accidents. private: - // Factory templates need to use the raw enum instead of the class wrapper. - using KindTemplateEnum = Internal::SemanticsNodeKindRawEnum; - // Provides Make and Get to support 0, 1, or 2 arguments for a Node. // These are protected so that child factories can opt in to what pieces they // want to use. - template + template class FactoryBase { protected: static auto Make(ParseTree::Node parse_node, TypeId type_id, @@ -273,7 +270,7 @@ class Node { }; // Provide Get along with a Make that requires a type. - template + template class Factory : public FactoryBase { public: using FactoryBase::Make; @@ -282,7 +279,7 @@ class Node { // Provides Get along with a Make that assumes the node doesn't produce a // typed value. - template + template class FactoryNoType : public FactoryBase { public: static auto Make(ParseTree::Node parse_node, ArgTypes... args) { @@ -436,9 +433,7 @@ class Node { // Gets the type of the value produced by evaluating this node. auto type_id() const -> TypeId { return type_id_; } - friend auto operator<<(llvm::raw_ostream& out, const Node& node) - -> llvm::raw_ostream&; - LLVM_DUMP_METHOD void Dump() const { llvm::errs() << *this; } + auto Print(llvm::raw_ostream& out) const -> void; private: // Builtins have peculiar construction, so they are a friend rather than using diff --git a/toolchain/semantics/semantics_node_kind.cpp b/toolchain/semantics/semantics_node_kind.cpp index 00c5bb4d5524..9b2c452e491b 100644 --- a/toolchain/semantics/semantics_node_kind.cpp +++ b/toolchain/semantics/semantics_node_kind.cpp @@ -4,15 +4,15 @@ #include "toolchain/semantics/semantics_node_kind.h" -namespace Carbon { +namespace Carbon::SemIR { -CARBON_DEFINE_ENUM_CLASS_NAMES(SemanticsNodeKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(NodeKind) = { #define CARBON_SEMANTICS_NODE_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name) #include "toolchain/semantics/semantics_node_kind.def" }; // Returns the name to use for this node kind in Semantics IR. -[[nodiscard]] auto SemanticsNodeKind::ir_name() const -> llvm::StringRef { +[[nodiscard]] auto NodeKind::ir_name() const -> llvm::StringRef { static constexpr llvm::StringRef Table[] = { #define CARBON_SEMANTICS_NODE_KIND_WITH_IR_NAME(Name, IR_Name) IR_Name, #include "toolchain/semantics/semantics_node_kind.def" @@ -20,22 +20,22 @@ CARBON_DEFINE_ENUM_CLASS_NAMES(SemanticsNodeKind) = { return Table[AsInt()]; } -auto SemanticsNodeKind::value_kind() const -> SemIR::NodeValueKind { - static constexpr SemIR::NodeValueKind Table[] = { +auto NodeKind::value_kind() const -> NodeValueKind { + static constexpr NodeValueKind Table[] = { #define CARBON_SEMANTICS_NODE_KIND_WITH_VALUE_KIND(Name, Kind) \ - SemIR::NodeValueKind::Kind, + NodeValueKind::Kind, #include "toolchain/semantics/semantics_node_kind.def" }; return Table[AsInt()]; } -auto SemanticsNodeKind::terminator_kind() const -> SemIR::TerminatorKind { - static constexpr SemIR::TerminatorKind Table[] = { +auto NodeKind::terminator_kind() const -> TerminatorKind { + static constexpr TerminatorKind Table[] = { #define CARBON_SEMANTICS_NODE_KIND_WITH_TERMINATOR_KIND(Name, Kind) \ - SemIR::TerminatorKind::Kind, + TerminatorKind::Kind, #include "toolchain/semantics/semantics_node_kind.def" }; return Table[AsInt()]; } -} // namespace Carbon +} // namespace Carbon::SemIR diff --git a/toolchain/semantics/semantics_node_kind.h b/toolchain/semantics/semantics_node_kind.h index 76cfcdaa1cff..7e5c2ad57ce4 100644 --- a/toolchain/semantics/semantics_node_kind.h +++ b/toolchain/semantics/semantics_node_kind.h @@ -38,16 +38,12 @@ enum class TerminatorKind : int8_t { Terminator, }; -} // namespace Carbon::SemIR - -namespace Carbon { - -CARBON_DEFINE_RAW_ENUM_CLASS(SemanticsNodeKind, uint8_t) { +CARBON_DEFINE_RAW_ENUM_CLASS(NodeKind, uint8_t) { #define CARBON_SEMANTICS_NODE_KIND(Name) CARBON_RAW_ENUM_ENUMERATOR(Name) #include "toolchain/semantics/semantics_node_kind.def" }; -class SemanticsNodeKind : public CARBON_ENUM_BASE(SemanticsNodeKind) { +class NodeKind : public CARBON_ENUM_BASE(NodeKind) { public: #define CARBON_SEMANTICS_NODE_KIND(Name) CARBON_ENUM_CONSTANT_DECLARATION(Name) #include "toolchain/semantics/semantics_node_kind.def" @@ -58,14 +54,14 @@ class SemanticsNodeKind : public CARBON_ENUM_BASE(SemanticsNodeKind) { [[nodiscard]] auto ir_name() const -> llvm::StringRef; // Returns whether this kind of node is expected to produce a value. - [[nodiscard]] auto value_kind() const -> SemIR::NodeValueKind; + [[nodiscard]] auto value_kind() const -> NodeValueKind; // Returns whether this node kind is a code block terminator, such as an // unconditional branch instruction, or part of the termination sequence, // such as a conditional branch instruction. The termination sequence of a // code block appears after all other instructions, and ends with a // terminator instruction. - [[nodiscard]] auto terminator_kind() const -> SemIR::TerminatorKind; + [[nodiscard]] auto terminator_kind() const -> TerminatorKind; // Compute a fingerprint for this node kind, allowing its use as part of the // key in a `FoldingSet`. @@ -73,17 +69,12 @@ class SemanticsNodeKind : public CARBON_ENUM_BASE(SemanticsNodeKind) { }; #define CARBON_SEMANTICS_NODE_KIND(Name) \ - CARBON_ENUM_CONSTANT_DEFINITION(SemanticsNodeKind, Name) + CARBON_ENUM_CONSTANT_DEFINITION(NodeKind, Name) #include "toolchain/semantics/semantics_node_kind.def" // We expect the node kind to fit compactly into 8 bits. -static_assert(sizeof(SemanticsNodeKind) == 1, "Kind objects include padding!"); +static_assert(sizeof(NodeKind) == 1, "Kind objects include padding!"); -// TODO: Refactor EnumBase to remove the need for this alias. -namespace SemIR { -using NodeKind = SemanticsNodeKind; -} // namespace SemIR - -} // namespace Carbon +} // namespace Carbon::SemIR #endif // CARBON_TOOLCHAIN_SEMANTICS_SEMANTICS_NODE_KIND_H_