NFC self-review: update comments

Author: Finn Plummer

clang/include/clang/Parse/ParseHLSLRootSignature.h

@@ -40,26 +40,31 @@ class RootSignatureParser {
 private:
   DiagnosticsEngine &getDiags() { return PP.getDiagnostics(); }
 
-  // All private Parse.* methods follow a similar pattern:
+  // All private parse.* methods follow a similar pattern:
   //   - Each method will start with an assert to denote what the CurToken is
   // expected to be and will parse from that token forward
   //
   //   - Therefore, it is the callers responsibility to ensure that you are
   // at the correct CurToken. This should be done with the pattern of:
   //
-  //  if (TryConsumeExpectedToken(RootSignatureToken::Kind))
-  //    if (Parse.*())
-  //      return true;
+  //  if (tryConsumeExpectedToken(RootSignatureToken::Kind)) {
+  //    auto ParsedObject = parse.*();
+  //    if (!ParsedObject.has_value())
+  //      return std::nullopt;
+  //    ...
+  // }
   //
   // or,
   //
-  //  if (ConsumeExpectedToken(RootSignatureToken::Kind, ...))
-  //    return true;
-  //  if (Parse.*())
-  //    return true;
+  //  if (consumeExpectedToken(RootSignatureToken::Kind, ...))
+  //    return std::nullopt;
+  //  auto ParsedObject = parse.*();
+  //  if (!ParsedObject.has_value())
+  //    return std::nullopt;
+  //  ...
   //
-  //   - All methods return true if a parsing error is encountered. It is the
-  // callers responsibility to propogate this error up, or deal with it
+  //   - All methods return std::nullopt if a parsing error is encountered. It
+  // is the callers responsibility to propogate this error up, or deal with it
   // otherwise
   //
   //   - An error will be raised if the proceeding tokens are not what is
@@ -70,16 +75,17 @@ class RootSignatureParser {
   std::optional<llvm::hlsl::rootsig::DescriptorTableClause>
   parseDescriptorTableClause();
 
-  struct ParsedParams {
+  /// Parameter parse methods
+  /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
+  /// order and only exactly once. `ParsedClauseParams` denotes the current
+  /// state of parsed params
+  struct ParsedClauseParams {
     std::optional<llvm::hlsl::rootsig::Register> Register;
     std::optional<uint32_t> Space;
   };
-  /// Parses out a `ParsedParams` for the caller to use for construction
-  /// of the in-memory representation of a Root Element.
-  std::optional<ParsedParams>
+  std::optional<ParsedClauseParams>
   parseDescriptorTableClauseParams(RootSignatureToken::Kind RegType);
 
-  /// Parameter parse methods corresponding to a ParamType
   std::optional<uint32_t> parseUIntParam();
   std::optional<llvm::hlsl::rootsig::Register> parseRegister();
 

clang/lib/Parse/ParseHLSLRootSignature.cpp

@@ -135,13 +135,17 @@ RootSignatureParser::parseDescriptorTableClause() {
   return Clause;
 }
 
-std::optional<RootSignatureParser::ParsedParams>
+std::optional<RootSignatureParser::ParsedClauseParams>
 RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
   assert(CurToken.TokKind == TokenKind::pu_l_paren &&
          "Expects to only be invoked starting at given token");
 
-  ParsedParams Params;
+  // Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
+  // order and only exactly once. Parse through as many arguments as possible
+  // reporting an error if a duplicate is seen.
+  ParsedClauseParams Params;
   do {
+    // ( `b` | `t` | `u` | `s`) POS_INT
     if (tryConsumeExpectedToken(RegType)) {
       if (Params.Register.has_value()) {
         getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
@@ -154,14 +158,17 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
       Params.Register = Reg;
     }
 
+    // `space` `=` POS_INT
     if (tryConsumeExpectedToken(TokenKind::kw_space)) {
       if (Params.Space.has_value()) {
         getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
+
       if (consumeExpectedToken(TokenKind::pu_equal))
         return std::nullopt;
+
       auto Space = parseUIntParam();
       if (!Space.has_value())
         return std::nullopt;