Refactor NodeCategory to provide a class API (#4004)

Mirroring #4003 for NodeCategory.

Note we template a lot more on NodeCategory's enum, so this is a
slightly more awkward delta.

Also, switch from Enum in KeywordModifierSet to RawEnumType for
consistency with EnumBase. The templating on NodeCategory had me
thinking about that more.

common/ostream.h

@@ -17,7 +17,7 @@
 namespace Carbon {
 
 // CRTP base class for printable types. Children (DerivedT) must implement:
-// - auto Print(llvm::raw_ostream& out) -> void
+// - auto Print(llvm::raw_ostream& out) const -> void
 template <typename DerivedT>
 class Printable {
   // Provides simple printing for debuggers.

toolchain/check/keyword_modifier_set.h

@@ -23,7 +23,7 @@ class KeywordModifierSet {
   //
   // We expect this to grow, so are using a bigger size than needed.
   // NOLINTNEXTLINE(performance-enum-size)
-  enum Enum : uint32_t {
+  enum RawEnumType : uint32_t {
     // At most one of these access modifiers allowed for a given declaration,
     // and if present it must be first:
     Private = 1 << 0,
@@ -61,7 +61,7 @@ class KeywordModifierSet {
   // Support implicit conversion so that the difference with the member enum is
   // opaque.
   // NOLINTNEXTLINE(google-explicit-constructor)
-  constexpr KeywordModifierSet(Enum set) : set_(set) {}
+  constexpr KeywordModifierSet(RawEnumType set) : set_(set) {}
 
   // Adds entries to the set.
   auto Add(KeywordModifierSet set) -> void { set_ |= set.set_; }
@@ -70,11 +70,11 @@ class KeywordModifierSet {
 
   // Returns true if there's a non-empty set intersection.
   constexpr auto HasAnyOf(KeywordModifierSet other) -> bool {
-    return !(*this & other).empty();
+    return set_ & other.set_;
   }
 
   // Returns true if empty.
-  constexpr auto empty() -> bool { return set_ == Enum::None; }
+  constexpr auto empty() -> bool { return !set_; }
 
   // Returns the set intersection.
   constexpr auto operator&(KeywordModifierSet other) -> KeywordModifierSet {
@@ -85,7 +85,7 @@ class KeywordModifierSet {
   auto operator~() -> KeywordModifierSet { return ~set_; }
 
  private:
-  Enum set_;
+  RawEnumType set_;
 };
 
 static_assert(!KeywordModifierSet(KeywordModifierSet::Access)

toolchain/check/node_stack.h

@@ -118,13 +118,13 @@ class NodeStack {
   // Returns whether the node on the top of the stack has an overlapping
   // category.
   auto PeekIs(Parse::NodeCategory category) const -> bool {
-    return !stack_.empty() && !!(PeekNodeKind().category() & category);
+    return !stack_.empty() && PeekNodeKind().category().HasAnyOf(category);
   }
 
   // Returns whether the node on the top of the stack has an overlapping
   // category. Templated for consistency with other functions taking a parse
   // node category.
-  template <Parse::NodeCategory RequiredParseCategory>
+  template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
   auto PeekIs() const -> bool {
     return PeekIs(RequiredParseCategory);
   }
@@ -216,7 +216,7 @@ class NodeStack {
   }
 
   // Pops the top of the stack and returns the node_id and the ID.
-  template <Parse::NodeCategory RequiredParseCategory>
+  template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
   auto PopWithNodeId() -> auto {
     auto id = Peek<RequiredParseCategory>();
     Parse::NodeIdInCategory<RequiredParseCategory> node_id(
@@ -242,7 +242,7 @@ class NodeStack {
   }
 
   // Pops the top of the stack and returns the ID.
-  template <Parse::NodeCategory RequiredParseCategory>
+  template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
   auto Pop() -> auto {
     return PopWithNodeId<RequiredParseCategory>().second;
   }
@@ -265,7 +265,7 @@ class NodeStack {
 
   // Pops the top of the stack if it has the given category, and returns the ID.
   // Otherwise returns std::nullopt.
-  template <Parse::NodeCategory RequiredParseCategory>
+  template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
   auto PopIf() -> std::optional<decltype(Pop<RequiredParseCategory>())> {
     if (PeekIs<RequiredParseCategory>()) {
       return Pop<RequiredParseCategory>();
@@ -286,7 +286,7 @@ class NodeStack {
 
   // Pops the top of the stack and returns the node_id and the ID if it is
   // of the specified category.
-  template <Parse::NodeCategory RequiredParseCategory>
+  template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
   auto PopWithNodeIdIf()
       -> std::pair<Parse::NodeIdInCategory<RequiredParseCategory>,
                    decltype(PopIf<RequiredParseCategory>())> {
@@ -314,7 +314,7 @@ class NodeStack {
   }
 
   // Peeks at the ID associated with the top of the name stack.
-  template <Parse::NodeCategory RequiredParseCategory>
+  template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
   auto Peek() const -> auto {
     Entry back = stack_.back();
     RequireParseCategory<RequiredParseCategory>(back.node_id);
@@ -358,14 +358,14 @@ class NodeStack {
       -> std::optional<Id::Kind> {
     std::optional<Id::Kind> result;
     auto set_id_if_category_is = [&](Parse::NodeCategory cat, Id::Kind kind) {
-      if (!!(category & cat)) {
+      if (category.HasAnyOf(cat)) {
         // Check for no consistent Id::Kind due to category with multiple bits
         // set. When computing the Id::Kind for a node kind, a partial category
         // match is OK, so long as we don't match two inconsistent categories.
         // When computing the Id::Kind for a category query, the query can't
         // have any extra bits set or we could be popping a node that is not in
         // this category.
-        if (for_node_kind ? result.has_value() : !!(category & ~cat)) {
+        if (for_node_kind ? result.has_value() : category.HasAnyOf(~cat)) {
           result = Id::Kind::Invalid;
         } else {
           result = kind;
@@ -515,10 +515,10 @@ class NodeStack {
   }
 
   // Require an entry to have the given Parse::NodeCategory.
-  template <Parse::NodeCategory RequiredParseCategory>
+  template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
   auto RequireParseCategory(Parse::NodeId node_id) const -> void {
     auto kind = parse_tree_->node_kind(node_id);
-    CARBON_CHECK(!!(RequiredParseCategory & kind.category()))
+    CARBON_CHECK(kind.category().HasAnyOf(RequiredParseCategory))
         << "Expected " << RequiredParseCategory << ", found " << kind
         << " with category " << kind.category();
   }

toolchain/parse/BUILD

@@ -25,6 +25,7 @@ cc_library(
     deps = [
         "//common:check",
         "//common:enum_base",
+        "//common:ostream",
         "//toolchain/base:index_base",
         "//toolchain/lex:token_kind",
         "@llvm-project//llvm:Support",

toolchain/parse/extract.cpp

@@ -96,7 +96,7 @@ static auto NodeIdInCategoryAccept(NodeCategory category, const Tree* tree,
                                    const Tree::SiblingIterator& it,
                                    Tree::SiblingIterator end,
                                    ErrorBuilder* trace) -> bool {
-  if (it == end || !(tree->node_kind(*it).category() & category)) {
+  if (it == end || !tree->node_kind(*it).category().HasAnyOf(category)) {
     if (trace) {
       *trace << "NodeIdInCategory " << category << " error: ";
       if (it == end) {
@@ -115,7 +115,7 @@ static auto NodeIdInCategoryAccept(NodeCategory category, const Tree* tree,
 }
 
 // Extract a `NodeIdInCategory<Category>` as a single child.
-template <NodeCategory Category>
+template <NodeCategory::RawEnumType Category>
 struct Extractable<NodeIdInCategory<Category>> {
   static auto Extract(const Tree* tree, Tree::SiblingIterator& it,
                       Tree::SiblingIterator end, ErrorBuilder* trace)

toolchain/parse/node_ids.h

@@ -50,14 +50,14 @@ const NodeKind& NodeIdForKind<K>::Kind = K;
 #include "toolchain/parse/node_kind.def"
 
 // NodeId that matches any NodeKind whose `category()` overlaps with `Category`.
-template <NodeCategory Category>
+template <NodeCategory::RawEnumType Category>
 struct NodeIdInCategory : public NodeId {
   // Support conversion from `NodeIdForKind<Kind>` if Kind's category
   // overlaps with `Category`.
   template <const NodeKind& Kind>
   // NOLINTNEXTLINE(google-explicit-constructor)
   NodeIdInCategory(NodeIdForKind<Kind> node_id) : NodeId(node_id) {
-    CARBON_CHECK(!!(Kind.category() & Category));
+    CARBON_CHECK(Kind.category().HasAnyOf(Category));
   }
 
   constexpr explicit NodeIdInCategory(NodeId node_id) : NodeId(node_id) {}

toolchain/parse/node_kind.cpp

@@ -10,16 +10,15 @@
 
 namespace Carbon::Parse {
 
-auto operator<<(llvm::raw_ostream& output, NodeCategory category)
-    -> llvm::raw_ostream& {
-  if (!category) {
-    output << "<none>";
+auto NodeCategory::Print(llvm::raw_ostream& out) const -> void {
+  if (!value_) {
+    out << "<none>";
   } else {
     llvm::ListSeparator sep("|");
 
-#define CARBON_NODE_CATEGORY(Name)         \
-  if (!!(category & NodeCategory::Name)) { \
-    output << sep << #Name;                \
+#define CARBON_NODE_CATEGORY(Name)   \
+  if (value_ & NodeCategory::Name) { \
+    out << sep << #Name;             \
   }
     CARBON_NODE_CATEGORY(Decl);
     CARBON_NODE_CATEGORY(Expr);
@@ -31,7 +30,6 @@ auto operator<<(llvm::raw_ostream& output, NodeCategory category)
     CARBON_NODE_CATEGORY(Statement);
 #undef CARBON_NODE_CATEGORY
   }
-  return output;
 }
 
 CARBON_DEFINE_ENUM_CLASS_NAMES(NodeKind) = {

toolchain/parse/node_kind.h

@@ -8,6 +8,7 @@
 #include <cstdint>
 
 #include "common/enum_base.h"
+#include "common/ostream.h"
 #include "llvm/ADT/BitmaskEnum.h"
 #include "toolchain/lex/token_kind.h"
 
@@ -16,30 +17,50 @@ namespace Carbon::Parse {
 LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
 
 // Represents a set of keyword modifiers, using a separate bit per modifier.
-//
-// We expect this to grow, so are using a bigger size than needed.
-// NOLINTNEXTLINE(performance-enum-size)
-enum class NodeCategory : uint32_t {
-  Decl = 1 << 0,
-  Expr = 1 << 1,
-  ImplAs = 1 << 2,
-  MemberExpr = 1 << 3,
-  MemberName = 1 << 4,
-  Modifier = 1 << 5,
-  Pattern = 1 << 6,
-  Statement = 1 << 7,
-  None = 0,
+class NodeCategory : public Printable<NodeCategory> {
+ public:
+  // Provide values as an enum. This doesn't expose these as NodeCategory
+  // instances just due to the duplication of declarations that would cause.
+  //
+  // We expect this to grow, so are using a bigger size than needed.
+  // NOLINTNEXTLINE(performance-enum-size)
+  enum RawEnumType : uint32_t {
+    Decl = 1 << 0,
+    Expr = 1 << 1,
+    ImplAs = 1 << 2,
+    MemberExpr = 1 << 3,
+    MemberName = 1 << 4,
+    Modifier = 1 << 5,
+    Pattern = 1 << 6,
+    Statement = 1 << 7,
+    None = 0,
 
-  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Statement)
+    LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Statement)
+  };
+
+  // Support implicit conversion so that the difference with the member enum is
+  // opaque.
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  constexpr NodeCategory(RawEnumType value) : value_(value) {}
+
+  // Returns true if there's a non-empty set intersection.
+  constexpr auto HasAnyOf(NodeCategory other) -> bool {
+    return value_ & other.value_;
+  }
+
+  // Returns the set inverse.
+  constexpr auto operator~() -> NodeCategory { return ~value_; }
+
+  auto operator==(const NodeCategory& other) const -> bool {
+    return value_ == other.value_;
+  }
+
+  auto Print(llvm::raw_ostream& out) const -> void;
+
+ private:
+  RawEnumType value_;
 };
 
-constexpr auto operator!(NodeCategory k) -> bool {
-  return !static_cast<uint32_t>(k);
-}
-
-auto operator<<(llvm::raw_ostream& output, NodeCategory category)
-    -> llvm::raw_ostream&;
-
 CARBON_DEFINE_RAW_ENUM_CLASS(NodeKind, uint8_t) {
 #define CARBON_PARSE_NODE_KIND(Name) CARBON_RAW_ENUM_ENUMERATOR(Name)
 #include "toolchain/parse/node_kind.def"

toolchain/parse/tree.h

@@ -525,10 +525,10 @@ struct Tree::ConvertTo<NodeIdForKind<K>> {
   static auto AllowedFor(NodeKind kind) -> bool { return kind == K; }
 };
 
-template <NodeCategory C>
+template <NodeCategory::RawEnumType C>
 struct Tree::ConvertTo<NodeIdInCategory<C>> {
   static auto AllowedFor(NodeKind kind) -> bool {
-    return !!(kind.category() & C);
+    return kind.category().HasAnyOf(C);
   }
 };
 

toolchain/parse/typed_nodes.h

@@ -27,7 +27,8 @@ template <typename Element, typename Comma>
 using CommaSeparatedList = llvm::SmallVector<ListItem<Element, Comma>>;
 
 // This class provides a shorthand for defining parse node kinds for leaf nodes.
-template <const NodeKind& KindT, NodeCategory Category = NodeCategory::None>
+template <const NodeKind& KindT,
+          NodeCategory::RawEnumType Category = NodeCategory::None>
 struct LeafNode {
   static constexpr auto Kind =
       KindT.Define({.category = Category, .child_count = 0});
@@ -314,7 +315,7 @@ struct ReturnType {
 };
 
 // A function signature: `fn F() -> i32`.
-template <const NodeKind& KindT, NodeCategory Category>
+template <const NodeKind& KindT, NodeCategory::RawEnumType Category>
 struct FunctionSignature {
   static constexpr auto Kind = KindT.Define(
       {.category = Category, .bracketed_by = FunctionIntroducer::Kind});
@@ -962,7 +963,7 @@ struct StructTypeLiteral {
 using ClassIntroducer = LeafNode<NodeKind::ClassIntroducer>;
 
 // A class signature `class C`
-template <const NodeKind& KindT, NodeCategory Category>
+template <const NodeKind& KindT, NodeCategory::RawEnumType Category>
 struct ClassSignature {
   static constexpr auto Kind = KindT.Define(
       {.category = Category, .bracketed_by = ClassIntroducer::Kind});
@@ -1027,7 +1028,7 @@ struct BaseDecl {
 using InterfaceIntroducer = LeafNode<NodeKind::InterfaceIntroducer>;
 
 // `interface I`
-template <const NodeKind& KindT, NodeCategory Category>
+template <const NodeKind& KindT, NodeCategory::RawEnumType Category>
 struct InterfaceSignature {
   static constexpr auto Kind = KindT.Define(
       {.category = Category, .bracketed_by = InterfaceIntroducer::Kind});
@@ -1080,7 +1081,7 @@ struct TypeImplAs {
 };
 
 // `impl T as I`
-template <const NodeKind& KindT, NodeCategory Category>
+template <const NodeKind& KindT, NodeCategory::RawEnumType Category>
 struct ImplSignature {
   static constexpr auto Kind = KindT.Define(
       {.category = Category, .bracketed_by = ImplIntroducer::Kind});
@@ -1115,7 +1116,7 @@ struct ImplDefinition {
 using NamedConstraintIntroducer = LeafNode<NodeKind::NamedConstraintIntroducer>;
 
 // `constraint NC`
-template <const NodeKind& KindT, NodeCategory Category>
+template <const NodeKind& KindT, NodeCategory::RawEnumType Category>
 struct NamedConstraintSignature {
   static constexpr auto Kind = KindT.Define(
       {.category = Category, .bracketed_by = NamedConstraintIntroducer::Kind});