review: prototype using a stateful struct for parsed params

pros:
- more explicit mapping to what parse method should be called based on
the current keyword/root element
- removes complexity of using `std::visit` on the parameter types
- allows for validations before the in-memory root element is
constructed

cons:
- we will need to bind the parsed values to the in-memory
representations for each element as opposed to just setting it directly.
but it makes the validations performed more explicit

notes:
- this does not enforce that we should do all validations after parsing
out, we do allow validations on the current token types (for instance
with register types)

Author: Finn Plummer

clang/include/clang/Parse/ParseHLSLRootSignature.h

@@ -69,45 +69,67 @@ class RootSignatureParser {
   bool parseDescriptorTable();
   bool parseDescriptorTableClause();
 
-  /// Each unique ParamType will have a custom parse method defined that can be
-  /// invoked to set a value to the referenced paramtype.
-  ///
-  /// This function will switch on the ParamType using std::visit and dispatch
-  /// onto the corresponding parse method
-  bool parseParam(llvm::hlsl::rootsig::ParamType Ref);
-
   /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
-  /// order, exactly once, and only a subset are mandatory. This function acts
-  /// as the infastructure to do so in a declarative way.
-  ///
-  /// For the example:
-  ///  SmallDenseMap<RootSignatureToken::Kind, ParamType> Params = {
-  ///    RootSignatureToken::Kind::bReg, &Clause.Register,
-  ///    RootSignatureToken::Kind::kw_space, &Clause.Space
-  ///  };
-  ///  SmallDenseSet<RootSignatureToken::Kind> Mandatory = {
-  ///    RootSignatureToken::Kind::bReg
-  ///  };
+  /// order and only exactly once. `ParsedParamState` provides a common
+  /// stateful structure to facilitate a common abstraction over collecting
+  /// parameters. By having a common return type we can follow clang's pattern
+  /// of first parsing out the values and then validating when we construct
+  /// the corresponding in-memory RootElements.
+  struct ParsedParamState {
+    // Parameter state to hold the parsed values. The value is only guarentted
+    // to be correct if one of its keyword bits is set in `Seen`.
+    // Eg) if any of the seen bits for `bReg, tReg, uReg, sReg` are set, then
+    // Register will have an initialized value
+    llvm::hlsl::rootsig::Register Register;
+    uint32_t Space;
+
+    // Seen retains whether or not the corresponding keyword has been
+    // seen
+    uint32_t Seen = 0u;
+
+    // Valid keywords for the different parameter types and its corresponding
+    // parsed value
+    ArrayRef<RootSignatureToken::Kind> Keywords;
+
+    // Context of which RootElement is retained to dispatch on the correct
+    // parse method.
+    // Eg) If we encounter kw_flags, it will depend on Context
+    // we are parsing to determine which `parse*Flags` method to call
+    RootSignatureToken::Kind Context;
+
+    // Retain start of params for reporting diagnostics
+    SourceLocation StartLoc;
+
+    // Must provide the starting context of the parameters
+    ParsedParamState(ArrayRef<RootSignatureToken::Kind> Keywords,
+                     RootSignatureToken::Kind Context, SourceLocation StartLoc)
+        : Keywords(Keywords), Context(Context), StartLoc(StartLoc) {}
+
+    // Helper functions to interact with Seen
+    size_t getKeywordIdx(RootSignatureToken::Kind Keyword);
+    bool checkAndSetSeen(RootSignatureToken::Kind Keyword);
+    bool checkAndClearSeen(RootSignatureToken::Kind Keyword);
+  };
+
+  /// Root signature parameters follow the form of `keyword` `=` `value`, or,
+  /// are a register. Given a keyword and the context of which `RootElement`
+  /// type we are parsing, we can dispatch onto the correct parseMethod to
+  /// parse a value into the `ParsedParamState`.
   ///
-  /// We can read it is as:
-  ///
-  /// when 'b0' is encountered, invoke the parse method for the type
-  ///   of &Clause.Register (Register *) and update the parameter
-  /// when 'space' is encountered, invoke a parse method for the type
-  ///   of &Clause.Space (uint32_t *) and update the parameter
-  ///
-  /// and 'bReg' must be specified
-  bool parseParams(llvm::SmallDenseMap<RootSignatureToken::Kind,
-                                       llvm::hlsl::rootsig::ParamType> &Params,
-                   llvm::SmallDenseSet<RootSignatureToken::Kind> &Mandatory);
+  /// This function implements the dispatch onto the correct parse method.
+  bool parseParam(ParsedParamState &Params);
+
+  /// Parses out a `ParsedParamState` for the caller to use for construction
+  /// of the in-memory representation of a Root Element.
+  bool parseParams(ParsedParamState &Params);
 
   /// Parameter parse methods corresponding to a ParamType
-  bool parseUIntParam(uint32_t *X);
-  bool parseRegister(llvm::hlsl::rootsig::Register *Reg);
+  bool parseUIntParam(uint32_t &X);
+  bool parseRegister(llvm::hlsl::rootsig::Register &Reg);
 
   /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned
   /// 32-bit integer
-  bool handleUIntLiteral(uint32_t *X);
+  bool handleUIntLiteral(uint32_t &X);
 
   /// Invoke the Lexer to consume a token and update CurToken with the result
   void consumeNextToken() { CurToken = Lexer.ConsumeToken(); }

clang/lib/Parse/ParseHLSLRootSignature.cpp

@@ -117,16 +117,26 @@ bool RootSignatureParser::parseDescriptorTableClause() {
                            ParamKind))
     return true;
 
-  llvm::SmallDenseMap<TokenKind, ParamType> Params = {
-      {ExpectedRegister, &Clause.Register},
-      {TokenKind::kw_space, &Clause.Space},
-  };
-  llvm::SmallDenseSet<TokenKind> Mandatory = {
+  TokenKind Keywords[2] = {
       ExpectedRegister,
+      TokenKind::kw_space,
   };
+  ParsedParamState Params(Keywords, ParamKind, CurToken.TokLoc);
+  if (parseParams(Params))
+    return true;
 
-  if (parseParams(Params, Mandatory))
+  // Mandatory parameters:
+  if (!Params.checkAndClearSeen(ExpectedRegister)) {
+    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+        << ExpectedRegister;
     return true;
+  }
+
+  Clause.Register = Params.Register;
+
+  // Optional parameters:
+  if (Params.checkAndClearSeen(TokenKind::kw_space))
+    Clause.Space = Params.Space;
 
   if (consumeExpectedToken(TokenKind::pu_r_paren,
                            diag::err_hlsl_unexpected_end_of_params,
@@ -137,68 +147,71 @@ bool RootSignatureParser::parseDescriptorTableClause() {
   return false;
 }
 
-// Helper struct defined to use the overloaded notation of std::visit.
-template <class... Ts> struct ParseParamTypeMethods : Ts... {
-  using Ts::operator()...;
-};
-template <class... Ts>
-ParseParamTypeMethods(Ts...) -> ParseParamTypeMethods<Ts...>;
-
-bool RootSignatureParser::parseParam(ParamType Ref) {
-  return std::visit(
-      ParseParamTypeMethods{
-          [this](Register *X) -> bool { return parseRegister(X); },
-          [this](uint32_t *X) -> bool {
-            return consumeExpectedToken(TokenKind::pu_equal,
-                                        diag::err_expected_after,
-                                        CurToken.TokKind) ||
-                   parseUIntParam(X);
-          },
-      },
-      Ref);
+size_t RootSignatureParser::ParsedParamState::getKeywordIdx(
+    RootSignatureToken::Kind Keyword) {
+  ArrayRef KeywordRef = Keywords;
+  auto It = llvm::find(KeywordRef, Keyword);
+  assert(It != KeywordRef.end() && "Did not provide a valid param keyword");
+  return std::distance(KeywordRef.begin(), It);
 }
 
-bool RootSignatureParser::parseParams(
-    llvm::SmallDenseMap<TokenKind, ParamType> &Params,
-    llvm::SmallDenseSet<TokenKind> &Mandatory) {
+bool RootSignatureParser::ParsedParamState::checkAndSetSeen(
+    RootSignatureToken::Kind Keyword) {
+  size_t Idx = getKeywordIdx(Keyword);
+  bool WasSeen = Seen & (1 << Idx);
+  Seen |= 1u << Idx;
+  return WasSeen;
+}
 
-  // Initialize a vector of possible keywords
-  SmallVector<TokenKind> Keywords;
-  for (auto Pair : Params)
-    Keywords.push_back(Pair.first);
+bool RootSignatureParser::ParsedParamState::checkAndClearSeen(
+    RootSignatureToken::Kind Keyword) {
+  size_t Idx = getKeywordIdx(Keyword);
+  bool WasSeen = Seen & (1 << Idx);
+  Seen &= ~(1u << Idx);
+  return WasSeen;
+}
 
-  // Keep track of which keywords have been seen to report duplicates
-  llvm::SmallDenseSet<TokenKind> Seen;
+bool RootSignatureParser::parseParam(ParsedParamState &Params) {
+  TokenKind Keyword = CurToken.TokKind;
+  if (Keyword == TokenKind::bReg || Keyword == TokenKind::tReg ||
+      Keyword == TokenKind::uReg || Keyword == TokenKind::sReg) {
+    return parseRegister(Params.Register);
+  }
+
+  if (consumeExpectedToken(TokenKind::pu_equal, diag::err_expected_after,
+                           Keyword))
+    return true;
 
-  while (tryConsumeExpectedToken(Keywords)) {
-    if (Seen.contains(CurToken.TokKind)) {
+  switch (Keyword) {
+  case RootSignatureToken::Kind::kw_space:
+    return parseUIntParam(Params.Space);
+  default:
+    llvm_unreachable("Switch for consumed keyword was not provided");
+  }
+}
+
+bool RootSignatureParser::parseParams(ParsedParamState &Params) {
+  assert(CurToken.TokKind == TokenKind::pu_l_paren &&
+         "Expects to only be invoked starting at given token");
+
+  while (tryConsumeExpectedToken(Params.Keywords)) {
+    if (Params.checkAndSetSeen(CurToken.TokKind)) {
       getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
           << CurToken.TokKind;
       return true;
     }
-    Seen.insert(CurToken.TokKind);
 
-    if (parseParam(Params[CurToken.TokKind]))
+    if (parseParam(Params))
       return true;
 
     if (!tryConsumeExpectedToken(TokenKind::pu_comma))
       break;
   }
 
-  bool AllMandatoryDefined = true;
-  for (auto Kind : Mandatory) {
-    bool SeenParam = Seen.contains(Kind);
-    if (!SeenParam) {
-      getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
-          << Kind;
-    }
-    AllMandatoryDefined &= SeenParam;
-  }
-
-  return !AllMandatoryDefined;
+  return false;
 }
 
-bool RootSignatureParser::parseUIntParam(uint32_t *X) {
+bool RootSignatureParser::parseUIntParam(uint32_t &X) {
   assert(CurToken.TokKind == TokenKind::pu_equal &&
          "Expects to only be invoked starting at given keyword");
   tryConsumeExpectedToken(TokenKind::pu_plus);
@@ -207,7 +220,7 @@ bool RootSignatureParser::parseUIntParam(uint32_t *X) {
          handleUIntLiteral(X);
 }
 
-bool RootSignatureParser::parseRegister(Register *Register) {
+bool RootSignatureParser::parseRegister(Register &Register) {
   assert((CurToken.TokKind == TokenKind::bReg ||
           CurToken.TokKind == TokenKind::tReg ||
           CurToken.TokKind == TokenKind::uReg ||
@@ -218,26 +231,26 @@ bool RootSignatureParser::parseRegister(Register *Register) {
   default:
     llvm_unreachable("Switch for consumed token was not provided");
   case TokenKind::bReg:
-    Register->ViewType = RegisterType::BReg;
+    Register.ViewType = RegisterType::BReg;
     break;
   case TokenKind::tReg:
-    Register->ViewType = RegisterType::TReg;
+    Register.ViewType = RegisterType::TReg;
     break;
   case TokenKind::uReg:
-    Register->ViewType = RegisterType::UReg;
+    Register.ViewType = RegisterType::UReg;
     break;
   case TokenKind::sReg:
-    Register->ViewType = RegisterType::SReg;
+    Register.ViewType = RegisterType::SReg;
     break;
   }
 
-  if (handleUIntLiteral(&Register->Number))
+  if (handleUIntLiteral(Register.Number))
     return true; // propogate NumericLiteralParser error
 
   return false;
 }
 
-bool RootSignatureParser::handleUIntLiteral(uint32_t *X) {
+bool RootSignatureParser::handleUIntLiteral(uint32_t &X) {
   // Parse the numeric value and do semantic checks on its specification
   clang::NumericLiteralParser Literal(CurToken.NumSpelling, CurToken.TokLoc,
                                       PP.getSourceManager(), PP.getLangOpts(),
@@ -256,7 +269,7 @@ bool RootSignatureParser::handleUIntLiteral(uint32_t *X) {
     return true;
   }
 
-  *X = Val.getExtValue();
+  X = Val.getExtValue();
   return false;
 }