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:
#6053 (comment)
- 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:
#6053 (comment)
- 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 <[email protected]>

Author: jonmeow

.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']

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"],

common/enum_base.h

@@ -5,6 +5,7 @@
 #ifndef CARBON_COMMON_ENUM_BASE_H_
 #define CARBON_COMMON_ENUM_BASE_H_
 
+#include <compare>
 #include <type_traits>
 
 #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<DerivedT> {
   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<DerivedT> {
 // `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.

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) {

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 <typename DerivedT, typename EnumT, const llvm::StringLiteral Names[]>
 class EnumMaskBase : public EnumBase<DerivedT, EnumT, Names> {
@@ -67,6 +70,8 @@ class EnumMaskBase : public EnumBase<DerivedT, EnumT, Names> {
   // 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<DerivedT, EnumT, Names> {
     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<DerivedT, EnumT, Names>::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 {                                                  \

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 <gtest/gtest.h>
+
+#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

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"
 };

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;

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

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"
 };