[NFC][RootSignatures] Conform to new std::optional calling conventions #136747
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-hlsl Author: Finn Plummer (inbelic) Changes
This is a clean-up task from #133800 Full diff: https://github.com/llvm/llvm-project/pull/136747.diff 2 Files Affected:
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 3eb3f8ea8422d..d9f121030c1fc 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -71,8 +71,9 @@ class RootSignatureParser {
// expected, or, there is a lexing error
/// Root Element parse methods:
- bool parseDescriptorTable();
- bool parseDescriptorTableClause();
+ std::optional<llvm::hlsl::rootsig::DescriptorTable> parseDescriptorTable();
+ std::optional<llvm::hlsl::rootsig::DescriptorTableClause>
+ parseDescriptorTableClause();
/// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
/// order and only exactly once. `ParsedClauseParams` denotes the current
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 4f8bfccfa2243..1bf33b8e8329c 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -26,22 +26,14 @@ RootSignatureParser::RootSignatureParser(SmallVector<RootElement> &Elements,
bool RootSignatureParser::parse() {
// Iterate as many RootElements as possible
- while (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
- // Dispatch onto parser method.
- // We guard against the unreachable here as we just ensured that CurToken
- // will be one of the kinds in the while condition
- switch (CurToken.TokKind) {
- case TokenKind::kw_DescriptorTable:
- if (parseDescriptorTable())
+ do {
+ if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
+ auto Table = parseDescriptorTable();
+ if (!Table.has_value())
return true;
- break;
- default:
- llvm_unreachable("Switch for consumed token was not provided");
+ Elements.push_back(*Table);
}
-
- if (!tryConsumeExpectedToken(TokenKind::pu_comma))
- break;
- }
+ } while (tryConsumeExpectedToken(TokenKind::pu_comma));
if (consumeExpectedToken(TokenKind::end_of_stream,
diag::err_hlsl_unexpected_end_of_params,
@@ -51,38 +43,38 @@ bool RootSignatureParser::parse() {
return false;
}
-bool RootSignatureParser::parseDescriptorTable() {
+std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
assert(CurToken.TokKind == TokenKind::kw_DescriptorTable &&
"Expects to only be invoked starting at given keyword");
- DescriptorTable Table;
-
if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
CurToken.TokKind))
- return true;
-
- // Iterate as many Clauses as possible
- while (tryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
- TokenKind::kw_UAV, TokenKind::kw_Sampler})) {
- if (parseDescriptorTableClause())
- return true;
+ return std::nullopt;
- Table.NumClauses++;
+ DescriptorTable Table;
- if (!tryConsumeExpectedToken(TokenKind::pu_comma))
- break;
- }
+ // Iterate as many Clauses as possible
+ do {
+ if (tryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
+ TokenKind::kw_UAV, TokenKind::kw_Sampler})) {
+ auto Clause = parseDescriptorTableClause();
+ if (!Clause.has_value())
+ return std::nullopt;
+ Elements.push_back(*Clause);
+ Table.NumClauses++;
+ }
+ } while (tryConsumeExpectedToken(TokenKind::pu_comma));
if (consumeExpectedToken(TokenKind::pu_r_paren,
diag::err_hlsl_unexpected_end_of_params,
/*param of=*/TokenKind::kw_DescriptorTable))
- return true;
+ return std::nullopt;
- Elements.push_back(Table);
- return false;
+ return Table;
}
-bool RootSignatureParser::parseDescriptorTableClause() {
+std::optional<DescriptorTableClause>
+RootSignatureParser::parseDescriptorTableClause() {
assert((CurToken.TokKind == TokenKind::kw_CBV ||
CurToken.TokKind == TokenKind::kw_SRV ||
CurToken.TokKind == TokenKind::kw_UAV ||
@@ -93,7 +85,7 @@ bool RootSignatureParser::parseDescriptorTableClause() {
if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
CurToken.TokKind))
- return true;
+ return std::nullopt;
DescriptorTableClause Clause;
TokenKind ExpectedReg;
@@ -120,13 +112,13 @@ bool RootSignatureParser::parseDescriptorTableClause() {
auto Params = parseDescriptorTableClauseParams(ExpectedReg);
if (!Params.has_value())
- return true;
+ return std::nullopt;
// Check mandatory parameters were provided
if (!Params->Reg.has_value()) {
getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
<< ExpectedReg;
- return true;
+ return std::nullopt;
}
Clause.Reg = Params->Reg.value();
@@ -138,10 +130,9 @@ bool RootSignatureParser::parseDescriptorTableClause() {
if (consumeExpectedToken(TokenKind::pu_r_paren,
diag::err_hlsl_unexpected_end_of_params,
/*param of=*/ParamKind))
- return true;
+ return std::nullopt;
- Elements.push_back(Clause);
- return false;
+ return Clause;
}
std::optional<RootSignatureParser::ParsedClauseParams>
|
|
@llvm/pr-subscribers-clang Author: Finn Plummer (inbelic) Changes
This is a clean-up task from #133800 Full diff: https://github.com/llvm/llvm-project/pull/136747.diff 2 Files Affected:
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 3eb3f8ea8422d..d9f121030c1fc 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -71,8 +71,9 @@ class RootSignatureParser {
// expected, or, there is a lexing error
/// Root Element parse methods:
- bool parseDescriptorTable();
- bool parseDescriptorTableClause();
+ std::optional<llvm::hlsl::rootsig::DescriptorTable> parseDescriptorTable();
+ std::optional<llvm::hlsl::rootsig::DescriptorTableClause>
+ parseDescriptorTableClause();
/// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
/// order and only exactly once. `ParsedClauseParams` denotes the current
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 4f8bfccfa2243..1bf33b8e8329c 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -26,22 +26,14 @@ RootSignatureParser::RootSignatureParser(SmallVector<RootElement> &Elements,
bool RootSignatureParser::parse() {
// Iterate as many RootElements as possible
- while (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
- // Dispatch onto parser method.
- // We guard against the unreachable here as we just ensured that CurToken
- // will be one of the kinds in the while condition
- switch (CurToken.TokKind) {
- case TokenKind::kw_DescriptorTable:
- if (parseDescriptorTable())
+ do {
+ if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
+ auto Table = parseDescriptorTable();
+ if (!Table.has_value())
return true;
- break;
- default:
- llvm_unreachable("Switch for consumed token was not provided");
+ Elements.push_back(*Table);
}
-
- if (!tryConsumeExpectedToken(TokenKind::pu_comma))
- break;
- }
+ } while (tryConsumeExpectedToken(TokenKind::pu_comma));
if (consumeExpectedToken(TokenKind::end_of_stream,
diag::err_hlsl_unexpected_end_of_params,
@@ -51,38 +43,38 @@ bool RootSignatureParser::parse() {
return false;
}
-bool RootSignatureParser::parseDescriptorTable() {
+std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
assert(CurToken.TokKind == TokenKind::kw_DescriptorTable &&
"Expects to only be invoked starting at given keyword");
- DescriptorTable Table;
-
if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
CurToken.TokKind))
- return true;
-
- // Iterate as many Clauses as possible
- while (tryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
- TokenKind::kw_UAV, TokenKind::kw_Sampler})) {
- if (parseDescriptorTableClause())
- return true;
+ return std::nullopt;
- Table.NumClauses++;
+ DescriptorTable Table;
- if (!tryConsumeExpectedToken(TokenKind::pu_comma))
- break;
- }
+ // Iterate as many Clauses as possible
+ do {
+ if (tryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
+ TokenKind::kw_UAV, TokenKind::kw_Sampler})) {
+ auto Clause = parseDescriptorTableClause();
+ if (!Clause.has_value())
+ return std::nullopt;
+ Elements.push_back(*Clause);
+ Table.NumClauses++;
+ }
+ } while (tryConsumeExpectedToken(TokenKind::pu_comma));
if (consumeExpectedToken(TokenKind::pu_r_paren,
diag::err_hlsl_unexpected_end_of_params,
/*param of=*/TokenKind::kw_DescriptorTable))
- return true;
+ return std::nullopt;
- Elements.push_back(Table);
- return false;
+ return Table;
}
-bool RootSignatureParser::parseDescriptorTableClause() {
+std::optional<DescriptorTableClause>
+RootSignatureParser::parseDescriptorTableClause() {
assert((CurToken.TokKind == TokenKind::kw_CBV ||
CurToken.TokKind == TokenKind::kw_SRV ||
CurToken.TokKind == TokenKind::kw_UAV ||
@@ -93,7 +85,7 @@ bool RootSignatureParser::parseDescriptorTableClause() {
if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
CurToken.TokKind))
- return true;
+ return std::nullopt;
DescriptorTableClause Clause;
TokenKind ExpectedReg;
@@ -120,13 +112,13 @@ bool RootSignatureParser::parseDescriptorTableClause() {
auto Params = parseDescriptorTableClauseParams(ExpectedReg);
if (!Params.has_value())
- return true;
+ return std::nullopt;
// Check mandatory parameters were provided
if (!Params->Reg.has_value()) {
getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
<< ExpectedReg;
- return true;
+ return std::nullopt;
}
Clause.Reg = Params->Reg.value();
@@ -138,10 +130,9 @@ bool RootSignatureParser::parseDescriptorTableClause() {
if (consumeExpectedToken(TokenKind::pu_r_paren,
diag::err_hlsl_unexpected_end_of_params,
/*param of=*/ParamKind))
- return true;
+ return std::nullopt;
- Elements.push_back(Clause);
- return false;
+ return Clause;
}
std::optional<RootSignatureParser::ParsedClauseParams>
|
|
Is this ready to retarget to main? |
bogner
approved these changes
Apr 23, 2025
damyanp
reviewed
Apr 23, 2025
Comment on lines
38
to
40
| if (consumeExpectedToken(TokenKind::end_of_stream, | ||
| diag::err_hlsl_unexpected_end_of_params, | ||
| /*param of=*/TokenKind::kw_RootSignature)) |
There was a problem hiding this comment.
Suggested change
| if (consumeExpectedToken(TokenKind::end_of_stream, | |
| diag::err_hlsl_unexpected_end_of_params, | |
| /*param of=*/TokenKind::kw_RootSignature)) | |
| return consumeExpectedToken(TokenKind::end_of_stream, | |
| diag::err_hlsl_unexpected_end_of_params, | |
| /*param of=*/TokenKind::kw_RootSignature); |
There was a problem hiding this comment.
Whoops, good catch. This doesn't relate to this pr but I have it noted to update when we touch the code in the future. Tracked here: #126576
- It was determined to define the parsing methods much more inline with a recursive descent parser to follow the EBNF notation better - As part of this change, we decided to go with a calling convention to the parse.* methods of returning an optional rather than a bool and a reference to the parsed struct
inbelic
force-pushed
the
inbelic/rs-dt-call-convs
branch
from
cb85ab7 to
50730f2
Compare
4 tasks
V-FEXrt
reviewed
Apr 23, 2025
| do { | ||
| if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) { | ||
| auto Table = parseDescriptorTable(); | ||
| if (!Table.has_value()) |
There was a problem hiding this comment.
nit: std::optional should already implement operator bool()
Suggested change
| if (!Table.has_value()) | |
| if (!Table) |
There was a problem hiding this comment.
Looks like there are a bunch of cases like this, I'll just leave the one comment but please update everywhere
There was a problem hiding this comment.
I will merge as-is for now since this explicit version is already established in the file.
I can create a clean-up pr later to adjust this throughout the whole file.
V-FEXrt
approved these changes
Apr 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a clean-up task from #133800