[HLSL][RootSignature] Use "stringified" version for diagnostic output of punctuator tokens

[inbelic]

This pr will corrects the output with a stringified version of a puncuator (eg ')') instead of its ascii value.

• Update LexHLSLRootSignature to fix the DiagnosticBuilder operator<< overload to correclty format the puncuator
• Add testcase demonstrating the stringified version is output

Resolves #145814.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Finn Plummer (inbelic)

Changes

This pr will corrects the output with a stringified version of a puncuator (eg ')') instead of its ascii value.

• Update LexHLSLRootSignature to fix the DiagnosticBuilder operator<< overload to correclty format the puncuator
• Add testcase demonstrating the stringified version is output

Resolves #145814.

---

Full diff: https://github.com/llvm/llvm-project/pull/145827.diff

2 Files Affected:

• (modified) clang/include/clang/Lex/LexHLSLRootSignature.h (+4)
• (modified) clang/test/SemaHLSL/RootSignature-err.hlsl (+4)

diff --git a/clang/include/clang/Lex/LexHLSLRootSignature.h b/clang/include/clang/Lex/LexHLSLRootSignature.h
index 9275e0d75840b..9901485b44d38 100644
--- a/clang/include/clang/Lex/LexHLSLRootSignature.h
+++ b/clang/include/clang/Lex/LexHLSLRootSignature.h
@@ -50,6 +50,10 @@ operator<<(const DiagnosticBuilder &DB, const RootSignatureToken::Kind Kind) {
   case RootSignatureToken::Kind::X:                                            \
     DB << SPELLING;                                                            \
     break;
+#define PUNCTUATOR(X, SPELLING)                                                \
+  case RootSignatureToken::Kind::pu_##X:                                       \
+    DB << #SPELLING;                                                           \
+    break;
 #include "clang/Lex/HLSLRootSignatureTokenKinds.def"
   }
   return DB;
diff --git a/clang/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl
index f544247f4db2a..aec8a15f8ed7f 100644
--- a/clang/test/SemaHLSL/RootSignature-err.hlsl
+++ b/clang/test/SemaHLSL/RootSignature-err.hlsl
@@ -18,3 +18,7 @@ void bad_root_signature_3() {}
 
 [RootSignature("DescriptorTable(), invalid")] // expected-error {{expected end of stream to denote end of parameters, or, another valid parameter of RootSignature}}
 void bad_root_signature_4() {}
+
+// expected-error@+1 {{expected ')' to denote end of parameters, or, another valid parameter of RootConstants}}
+[RootSignature("RootConstants(b0, num32BitConstants = 1, invalid)")]
+void bad_root_signature_5() {}

[llvmbot]

@llvm/pr-subscribers-hlsl

Author: Finn Plummer (inbelic)

Changes

This pr will corrects the output with a stringified version of a puncuator (eg ')') instead of its ascii value.

• Update LexHLSLRootSignature to fix the DiagnosticBuilder operator<< overload to correclty format the puncuator
• Add testcase demonstrating the stringified version is output

Resolves #145814.

---

Full diff: https://github.com/llvm/llvm-project/pull/145827.diff

2 Files Affected:

• (modified) clang/include/clang/Lex/LexHLSLRootSignature.h (+4)
• (modified) clang/test/SemaHLSL/RootSignature-err.hlsl (+4)

diff --git a/clang/include/clang/Lex/LexHLSLRootSignature.h b/clang/include/clang/Lex/LexHLSLRootSignature.h
index 9275e0d75840b..9901485b44d38 100644
--- a/clang/include/clang/Lex/LexHLSLRootSignature.h
+++ b/clang/include/clang/Lex/LexHLSLRootSignature.h
@@ -50,6 +50,10 @@ operator<<(const DiagnosticBuilder &DB, const RootSignatureToken::Kind Kind) {
   case RootSignatureToken::Kind::X:                                            \
     DB << SPELLING;                                                            \
     break;
+#define PUNCTUATOR(X, SPELLING)                                                \
+  case RootSignatureToken::Kind::pu_##X:                                       \
+    DB << #SPELLING;                                                           \
+    break;
 #include "clang/Lex/HLSLRootSignatureTokenKinds.def"
   }
   return DB;
diff --git a/clang/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl
index f544247f4db2a..aec8a15f8ed7f 100644
--- a/clang/test/SemaHLSL/RootSignature-err.hlsl
+++ b/clang/test/SemaHLSL/RootSignature-err.hlsl
@@ -18,3 +18,7 @@ void bad_root_signature_3() {}
 
 [RootSignature("DescriptorTable(), invalid")] // expected-error {{expected end of stream to denote end of parameters, or, another valid parameter of RootSignature}}
 void bad_root_signature_4() {}
+
+// expected-error@+1 {{expected ')' to denote end of parameters, or, another valid parameter of RootConstants}}
+[RootSignature("RootConstants(b0, num32BitConstants = 1, invalid)")]
+void bad_root_signature_5() {}

[V-FEXrt]

Am I missing a detail here? The parens are matched in this test case right? If so this error message would send me down a very wrong path.

Maybe that is because I'm not familiar with rootsigs, maybe someone more familiar with them would understand the error better

[V-FEXrt]

also I'm aware this comment isn't related to the actual fix of the PR so it's not blocking, just a side point

[inbelic]

Thank you for the feedback, when you work with the code for months and write the diagnostic it is always makes sense to yourself.

Note: There is some missing info here, as it doesn't have where the diagnostic is pointing in the source location.
In this case it would be pointing right after the comma, and the desired interpretation would be:
"after this comma it is expected that we should either have a ')' to denote the end of parameters or another valid parameter of RootConstants".

With that being said, I am very open to feedback and I can see the confusion!

I will create a todo to take another look at clarifying the diagnostic messages.