From 973d7219166c69838a420aaf7bef636e9d8a6156 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 12 Sep 2025 15:59:37 -0700 Subject: [PATCH] Some more edits to EnumBase and EnumMaskBase (#6054) Adds a unit test, and some smaller edits: - Remove the `=` when defining names, in order to change `}` placement by clang-format on uses. - context: https://github.com/carbon-language/carbon-lang/pull/6053#discussion_r2343423178 - I believe with `EnumBase` that keeping the `=` had been a deliberate choice, so this PR is intended to confirm that removing it is okay. - Delete `EnumMaskBase::name` - context: https://github.com/carbon-language/carbon-lang/pull/6053#discussion_r2344233707 - We can't just do nothing because `EnumBase::name` uses indexing that's incompatible with `EnumMaskBase`. - Some small comment cleanups. - Tests don't need to be in the `Carbon` namespace anymore, macros work fine in other namespaces, but it's still the right namespace. - Documentation on `EnumBase::name` seems to be referring to a prior structure, wherein we had a macro defining the function instead of the `Names` array. --------- Co-authored-by: Richard Smith --- .clang-format | 2 + common/BUILD | 12 ++ common/enum_base.h | 17 ++- common/enum_base_test.cpp | 12 +- common/enum_mask_base.h | 35 ++--- common/enum_mask_base_test.cpp | 142 +++++++++++++++++++++ toolchain/base/llvm_tools.cpp | 2 +- toolchain/check/generic.cpp | 5 +- toolchain/check/keyword_modifier_set.cpp | 5 +- toolchain/diagnostics/diagnostic_kind.cpp | 2 +- toolchain/lex/token_kind.cpp | 2 +- toolchain/parse/node_category.cpp | 5 +- toolchain/parse/node_kind.cpp | 2 +- toolchain/parse/state.cpp | 2 +- toolchain/sem_ir/builtin_function_kind.cpp | 2 +- toolchain/sem_ir/inst_kind.cpp | 2 +- toolchain/sem_ir/type.cpp | 5 +- 17 files changed, 202 insertions(+), 52 deletions(-) create mode 100644 common/enum_mask_base_test.cpp diff --git a/.clang-format b/.clang-format index a1cb60a4e88a..da34f9c2b51b 100644 --- a/.clang-format +++ b/.clang-format @@ -17,7 +17,9 @@ SpaceBeforeParens: ControlStatementsExceptControlMacros IfMacros: [ 'CARBON_DEFINE_RAW_ENUM_CLASS', + 'CARBON_DEFINE_ENUM_CLASS_NAMES', 'CARBON_DEFINE_RAW_ENUM_MASK', + 'CARBON_DEFINE_ENUM_MASK_NAMES', 'CARBON_KIND_SWITCH', ] StatementMacros: ['ABSTRACT'] diff --git a/common/BUILD b/common/BUILD index 98457b685416..b050aad51b7f 100644 --- a/common/BUILD +++ b/common/BUILD @@ -168,6 +168,18 @@ cc_library( ], ) +cc_test( + name = "enum_mask_base_test", + size = "small", + srcs = ["enum_mask_base_test.cpp"], + deps = [ + ":enum_mask_base", + ":raw_string_ostream", + "//testing/base:gtest_main", + "@googletest//:gtest", + ], +) + cc_library( name = "error", hdrs = ["error.h"], diff --git a/common/enum_base.h b/common/enum_base.h index d4053ec2f8d2..f4c805cc576a 100644 --- a/common/enum_base.h +++ b/common/enum_base.h @@ -5,6 +5,7 @@ #ifndef CARBON_COMMON_ENUM_BASE_H_ #define CARBON_COMMON_ENUM_BASE_H_ +#include #include #include "common/ostream.h" @@ -53,7 +54,7 @@ namespace Carbon::Internal { // // In `my_kind.cpp`: // ``` -// CARBON_DEFINE_ENUM_CLASS_NAMES(MyKind) = { +// CARBON_DEFINE_ENUM_CLASS_NAMES(MyKind) { // #define CARBON_MY_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name) // #include ".../my_kind.def" // }; @@ -129,15 +130,19 @@ class EnumBase : public Printable { explicit operator bool() const = delete; // Returns the name of this value. - // - // 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. auto name() const -> llvm::StringRef { return Names[AsInt()]; } // Prints this value using its name. auto Print(llvm::raw_ostream& out) const -> void { out << name(); } + // Don't support comparison of enums by default. + friend auto operator<(DerivedT lhs, DerivedT rhs) -> bool = delete; + friend auto operator<=(DerivedT lhs, DerivedT rhs) -> bool = delete; + friend auto operator>(DerivedT lhs, DerivedT rhs) -> bool = delete; + friend auto operator>=(DerivedT lhs, DerivedT rhs) -> bool = delete; + friend auto operator<=>(DerivedT lhs, DerivedT rhs) + -> std::partial_ordering = delete; + protected: // The default constructor is explicitly defaulted (and constexpr) as a // protected constructor to allow derived classes to be constructed but not @@ -215,7 +220,7 @@ class EnumBase : public Printable { // `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) \ - constexpr llvm::StringLiteral Internal::EnumClassName##Data::Names[] + constexpr llvm::StringLiteral Internal::EnumClassName##Data::Names[] = // Use this within the names array initializer to generate a string for each // name. diff --git a/common/enum_base_test.cpp b/common/enum_base_test.cpp index 6a68455fb56e..4dbe1ab34dcd 100644 --- a/common/enum_base_test.cpp +++ b/common/enum_base_test.cpp @@ -30,7 +30,7 @@ class TestKind : public CARBON_ENUM_BASE(TestKind) { CARBON_ENUM_CONSTANT_DEFINITION(TestKind, Name) #include "common/enum_base_test.def" -CARBON_DEFINE_ENUM_CLASS_NAMES(TestKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(TestKind) { #define CARBON_ENUM_BASE_TEST_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name) #include "common/enum_base_test.def" }; @@ -79,23 +79,15 @@ TEST(EnumBaseTest, Switch) { TEST(EnumBaseTest, Comparison) { TestKind kind = TestKind::Beep; - // Make sure all the different comparisons work, and also to work with + // Make sure all the different comparisons work, and also work with // GoogleTest expectations. EXPECT_EQ(TestKind::Beep, kind); EXPECT_NE(TestKind::Boop, kind); - EXPECT_LT(kind, TestKind::Boop); - EXPECT_GT(TestKind::Burr, kind); - EXPECT_LE(kind, TestKind::Beep); - EXPECT_GE(TestKind::Beep, kind); // These should also all be constexpr. constexpr TestKind Kind2 = TestKind::Beep; static_assert(Kind2 == TestKind::Beep); static_assert(Kind2 != TestKind::Boop); - static_assert(Kind2 < TestKind::Boop); - static_assert(!(Kind2 > TestKind::Burr)); - static_assert(Kind2 <= TestKind::Beep); - static_assert(!(Kind2 >= TestKind::Burr)); } TEST(EnumBaseTest, IntConversion) { diff --git a/common/enum_mask_base.h b/common/enum_mask_base.h index bc5279cf0a29..9c87f95f40d1 100644 --- a/common/enum_mask_base.h +++ b/common/enum_mask_base.h @@ -13,6 +13,8 @@ namespace Carbon::Internal { // CRTP-style base class similar to `EnumBase`, but supporting mask enums. +// Enumerator values are consecutive bit shifts (1 << 0, 1 << 1, 1 << 2, 1 << 3, +// ...). // // Users must be in the `Carbon` namespace and should look like the following. // @@ -24,7 +26,7 @@ namespace Carbon::Internal { // X(Enumerator3) \ // ... // -// CARBON_DEFINE_RAW_ENUM_MASK(MyKind) { +// CARBON_DEFINE_RAW_ENUM_MASK(MyKind, uint32_t) { // CARBON_MY_KIND(CARBON_RAW_ENUM_MASK_ENUMERATOR) // }; // @@ -43,8 +45,9 @@ namespace Carbon::Internal { // // In `my_kind.cpp`: // ``` -// CARBON_DEFINE_ENUM_MASK_NAMES(MyKind) = { -// CARBON_MY_KIND(CARBON_ENUM_MASK_NAME_STRING)}; +// CARBON_DEFINE_ENUM_MASK_NAMES(MyKind) { +// CARBON_MY_KIND(CARBON_ENUM_MASK_NAME_STRING) +// }; // ``` template class EnumMaskBase : public EnumBase { @@ -67,6 +70,8 @@ class EnumMaskBase : public EnumBase { // Removes entries from the mask. auto Remove(DerivedT other) -> void { *this = *this & ~other; } + constexpr auto empty() const -> bool { return this->AsInt() == 0; } + constexpr auto operator|(DerivedT other) const -> DerivedT { return DerivedT::FromInt(this->AsInt() | other.AsInt()); } @@ -79,21 +84,9 @@ class EnumMaskBase : public EnumBase { return DerivedT::FromInt(~this->AsInt()); } - constexpr auto empty() const -> bool { return this->AsInt() == 0; } - - // Returns the name of this value. Requires it to be a single value, not - // combined. - // - // 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. - // - // This shadows EnumBase::name. - auto name() const -> llvm::StringRef { - CARBON_CHECK(std::has_single_bit(this->AsInt()), "Not a single bit: {0}", - this->AsInt()); - return Names[std::bit_width(this->AsInt())]; - } + // Use `Print` for mask entries. This hides `EnumBase::name`; it's not + // compatible with `EnumMaskBase`. + auto name() const -> llvm::StringRef = delete; // Prints this value as a `|`-separated list of mask entries, or `None`. // @@ -119,9 +112,9 @@ constexpr const DerivedT& EnumMaskBase::None = } // namespace Carbon::Internal -// 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. +// Use this before defining a class that derives from `EnumMaskBase` 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_MASK(EnumMaskName, UnderlyingType) \ namespace Internal { \ struct EnumMaskName##Data { \ diff --git a/common/enum_mask_base_test.cpp b/common/enum_mask_base_test.cpp new file mode 100644 index 000000000000..4bc278402c9e --- /dev/null +++ b/common/enum_mask_base_test.cpp @@ -0,0 +1,142 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "common/enum_mask_base.h" + +#include + +#include "common/raw_string_ostream.h" + +namespace Carbon { +namespace { + +#define CARBON_TEST_KIND(X) \ + X(Beep) \ + X(Boop) \ + X(Burr) + +CARBON_DEFINE_RAW_ENUM_MASK(TestKind, uint8_t) { + CARBON_TEST_KIND(CARBON_RAW_ENUM_MASK_ENUMERATOR) +}; + +class TestKind : public CARBON_ENUM_MASK_BASE(TestKind) { + public: + CARBON_TEST_KIND(CARBON_ENUM_MASK_CONSTANT_DECL) + + using EnumMaskBase::AsInt; + using EnumMaskBase::FromInt; +}; + +#define CARBON_TEST_KIND_WITH_TYPE(X) \ + CARBON_ENUM_MASK_CONSTANT_DEFINITION(TestKind, X) +CARBON_TEST_KIND(CARBON_TEST_KIND_WITH_TYPE) +#undef CARBON_TEST_KIND_WITH_TYPE + +CARBON_DEFINE_ENUM_MASK_NAMES(TestKind) { + CARBON_TEST_KIND(CARBON_ENUM_MASK_NAME_STRING) +}; + +static_assert(sizeof(TestKind) == sizeof(uint8_t), + "Class size doesn't match enum size!"); + +TEST(EnumMaskBaseTest, Printing) { + RawStringOstream stream; + + TestKind kind = TestKind::Beep; + stream << kind; + EXPECT_EQ("Beep", stream.TakeStr()); + + kind = TestKind::Boop; + stream << kind; + EXPECT_EQ("Boop", stream.TakeStr()); + + stream << TestKind::Beep; + EXPECT_EQ("Beep", stream.TakeStr()); + + stream << (TestKind::Beep | TestKind::Burr); + EXPECT_EQ("Beep|Burr", stream.TakeStr()); +} + +// This just ensures it compiles, it's not validating what's printed. +TEST(EnumMaskBaseTest, PrintToGoogletest) { + EXPECT_TRUE(true) << TestKind::Beep; +} + +TEST(EnumMaskBaseTest, Switch) { + TestKind kind = TestKind::Boop; + + switch (kind) { + case TestKind::Beep: { + FAIL() << "Beep case selected!"; + break; + } + case TestKind::Boop: { + EXPECT_EQ(kind, TestKind::Boop); + break; + } + case TestKind::Burr: { + FAIL() << "Burr case selected!"; + break; + } + } +} + +TEST(EnumMaskBaseTest, Equality) { + TestKind kind = TestKind::Beep; + + // Make sure all the different comparisons work, and also work with + // GoogleTest expectations. + EXPECT_EQ(TestKind::Beep, kind); + EXPECT_NE(TestKind::Boop, kind); + + // These should also all be constexpr. + constexpr TestKind Kind2 = TestKind::Beep; + static_assert(Kind2 == TestKind::Beep); + static_assert(Kind2 != TestKind::Boop); +} + +TEST(EnumMaskBaseTest, AddRemove) { + TestKind kind = TestKind::Beep; + EXPECT_EQ(kind, TestKind::Beep); + kind.Add(TestKind::Beep); + EXPECT_EQ(kind, TestKind::Beep); + kind.Add(TestKind::Burr); + EXPECT_EQ(kind, TestKind::Beep | TestKind::Burr); + kind.Remove(TestKind::Beep); + EXPECT_EQ(kind, TestKind::Burr); + kind.Remove(TestKind::Beep); + EXPECT_EQ(kind, TestKind::Burr); + kind.Remove(TestKind::Burr); + EXPECT_EQ(kind, TestKind::None); +} + +TEST(EnumMaskBaseTest, HasAnyOf) { + static_assert(TestKind::Beep.HasAnyOf(TestKind::Beep)); + static_assert(TestKind::Beep.HasAnyOf(TestKind::Beep | TestKind::Burr)); + static_assert(!TestKind::Beep.HasAnyOf(TestKind::Burr)); +} + +TEST(EnumMaskBaseTest, MaskOperations) { + TestKind kind = + TestKind::Beep | (TestKind::Burr & (TestKind::Burr | TestKind::Beep)); + EXPECT_EQ(kind, TestKind::Beep | TestKind::Burr); + + // These should also all be constexpr. + static_assert((TestKind::Beep & TestKind::Burr) == TestKind::None); + static_assert((TestKind::Beep | TestKind::Burr) != TestKind::None); + static_assert(TestKind::Beep == ~~TestKind::Beep); +} + +TEST(EnumMaskBaseTest, IntConversion) { + EXPECT_EQ(1, TestKind::Beep.AsInt()); + EXPECT_EQ(2, TestKind::Boop.AsInt()); + EXPECT_EQ(4, TestKind::Burr.AsInt()); + + EXPECT_EQ(TestKind::Beep, TestKind::FromInt(1)); + EXPECT_EQ(TestKind::Boop, TestKind::FromInt(2)); + EXPECT_EQ(TestKind::Burr, TestKind::FromInt(4)); +} + +} // namespace +} // namespace Carbon diff --git a/toolchain/base/llvm_tools.cpp b/toolchain/base/llvm_tools.cpp index 13088fa600d0..4ae29221ce92 100644 --- a/toolchain/base/llvm_tools.cpp +++ b/toolchain/base/llvm_tools.cpp @@ -15,7 +15,7 @@ namespace Carbon { -CARBON_DEFINE_ENUM_CLASS_NAMES(LLVMTool) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(LLVMTool) { #define CARBON_LLVM_TOOL(Identifier, Name, BinName, MainFn) Name, #include "toolchain/base/llvm_tools.def" }; diff --git a/toolchain/check/generic.cpp b/toolchain/check/generic.cpp index 9769e4ca4068..7cc484cc9fb3 100644 --- a/toolchain/check/generic.cpp +++ b/toolchain/check/generic.cpp @@ -23,8 +23,9 @@ namespace Carbon::Check { -CARBON_DEFINE_ENUM_MASK_NAMES(DependentInstKind) = { - CARBON_DEPENDENT_INST_KIND(CARBON_ENUM_MASK_NAME_STRING)}; +CARBON_DEFINE_ENUM_MASK_NAMES(DependentInstKind) { + CARBON_DEPENDENT_INST_KIND(CARBON_ENUM_MASK_NAME_STRING) +}; static auto MakeSelfSpecificId(Context& context, SemIR::GenericId generic_id) -> SemIR::SpecificId; diff --git a/toolchain/check/keyword_modifier_set.cpp b/toolchain/check/keyword_modifier_set.cpp index 091ba97ee822..5d39d41e722f 100644 --- a/toolchain/check/keyword_modifier_set.cpp +++ b/toolchain/check/keyword_modifier_set.cpp @@ -6,7 +6,8 @@ namespace Carbon::Check { -CARBON_DEFINE_ENUM_MASK_NAMES(KeywordModifierSet) = { - CARBON_KEYWORD_MODIFIER_SET(CARBON_ENUM_MASK_NAME_STRING)}; +CARBON_DEFINE_ENUM_MASK_NAMES(KeywordModifierSet) { + CARBON_KEYWORD_MODIFIER_SET(CARBON_ENUM_MASK_NAME_STRING) +}; } // namespace Carbon::Check diff --git a/toolchain/diagnostics/diagnostic_kind.cpp b/toolchain/diagnostics/diagnostic_kind.cpp index 041e4f370980..790b52b3e483 100644 --- a/toolchain/diagnostics/diagnostic_kind.cpp +++ b/toolchain/diagnostics/diagnostic_kind.cpp @@ -6,7 +6,7 @@ namespace Carbon::Diagnostics { -CARBON_DEFINE_ENUM_CLASS_NAMES(Kind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(Kind) { #define CARBON_DIAGNOSTIC_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name) #include "toolchain/diagnostics/diagnostic_kind.def" }; diff --git a/toolchain/lex/token_kind.cpp b/toolchain/lex/token_kind.cpp index a36d86575cdf..66e0276fe8d4 100644 --- a/toolchain/lex/token_kind.cpp +++ b/toolchain/lex/token_kind.cpp @@ -6,7 +6,7 @@ namespace Carbon::Lex { -CARBON_DEFINE_ENUM_CLASS_NAMES(TokenKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(TokenKind) { #define CARBON_TOKEN(TokenName) CARBON_ENUM_CLASS_NAME_STRING(TokenName) #include "toolchain/lex/token_kind.def" }; diff --git a/toolchain/parse/node_category.cpp b/toolchain/parse/node_category.cpp index 49f236ddd061..c493b725d29f 100644 --- a/toolchain/parse/node_category.cpp +++ b/toolchain/parse/node_category.cpp @@ -8,7 +8,8 @@ namespace Carbon::Parse { -CARBON_DEFINE_ENUM_MASK_NAMES(NodeCategory) = { - CARBON_NODE_CATEGORY(CARBON_ENUM_MASK_NAME_STRING)}; +CARBON_DEFINE_ENUM_MASK_NAMES(NodeCategory) { + CARBON_NODE_CATEGORY(CARBON_ENUM_MASK_NAME_STRING) +}; } // namespace Carbon::Parse diff --git a/toolchain/parse/node_kind.cpp b/toolchain/parse/node_kind.cpp index e60a28aef80a..72f953e2d3ba 100644 --- a/toolchain/parse/node_kind.cpp +++ b/toolchain/parse/node_kind.cpp @@ -9,7 +9,7 @@ namespace Carbon::Parse { -CARBON_DEFINE_ENUM_CLASS_NAMES(NodeKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(NodeKind) { #define CARBON_PARSE_NODE_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name) #include "toolchain/parse/node_kind.def" }; diff --git a/toolchain/parse/state.cpp b/toolchain/parse/state.cpp index 7fcdcb4c17d6..b7d2d324cbad 100644 --- a/toolchain/parse/state.cpp +++ b/toolchain/parse/state.cpp @@ -6,7 +6,7 @@ namespace Carbon::Parse { -CARBON_DEFINE_ENUM_CLASS_NAMES(StateKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(StateKind) { #define CARBON_PARSE_STATE(Name) CARBON_ENUM_CLASS_NAME_STRING(Name) #include "toolchain/parse/state.def" }; diff --git a/toolchain/sem_ir/builtin_function_kind.cpp b/toolchain/sem_ir/builtin_function_kind.cpp index 432574bf92eb..a932812c44a7 100644 --- a/toolchain/sem_ir/builtin_function_kind.cpp +++ b/toolchain/sem_ir/builtin_function_kind.cpp @@ -603,7 +603,7 @@ constexpr BuiltinInfo TypeAnd = {"type.and", } // namespace BuiltinFunctionInfo -CARBON_DEFINE_ENUM_CLASS_NAMES(BuiltinFunctionKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(BuiltinFunctionKind) { #define CARBON_SEM_IR_BUILTIN_FUNCTION_KIND(Name) \ BuiltinFunctionInfo::Name.name, #include "toolchain/sem_ir/builtin_function_kind.def" diff --git a/toolchain/sem_ir/inst_kind.cpp b/toolchain/sem_ir/inst_kind.cpp index e71e6cdfc6a6..180a33deae1d 100644 --- a/toolchain/sem_ir/inst_kind.cpp +++ b/toolchain/sem_ir/inst_kind.cpp @@ -8,7 +8,7 @@ namespace Carbon::SemIR { -CARBON_DEFINE_ENUM_CLASS_NAMES(InstKind) = { +CARBON_DEFINE_ENUM_CLASS_NAMES(InstKind) { #define CARBON_SEM_IR_INST_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name) #include "toolchain/sem_ir/inst_kind.def" }; diff --git a/toolchain/sem_ir/type.cpp b/toolchain/sem_ir/type.cpp index 61030fc54ac8..0c16785a7680 100644 --- a/toolchain/sem_ir/type.cpp +++ b/toolchain/sem_ir/type.cpp @@ -10,8 +10,9 @@ namespace Carbon::SemIR { -CARBON_DEFINE_ENUM_MASK_NAMES(TypeQualifiers) = { - CARBON_TYPE_QUALIFIERS(CARBON_ENUM_MASK_NAME_STRING)}; +CARBON_DEFINE_ENUM_MASK_NAMES(TypeQualifiers) { + CARBON_TYPE_QUALIFIERS(CARBON_ENUM_MASK_NAME_STRING) +}; // Verify that the constant value's type is `TypeType` (or an error). static void CheckTypeOfConstantIsTypeType(File& file, ConstantId constant_id) {