Skip to content

Commit 74ff0c8

Browse files
inbelicinbelic
committed
improve diagnostics for static samplers
1 parent c6a2f4a commit 74ff0c8

File tree

2 files changed

+66
-54
lines changed

2 files changed

+66
-54
lines changed

‎clang/lib/Parse/ParseHLSLRootSignature.cpp

Lines changed: 39 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,9 @@ std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() {
374374
if (!Params.has_value())
375375
return std::nullopt;
376376

377+
if (consumeExpectedToken(TokenKind::pu_r_paren))
378+
return std::nullopt;
379+
377380
// Check mandatory parameters were provided
378381
if (!Params->Reg.has_value()) {
379382
reportDiag(diag::err_hlsl_rootsig_missing_param) << TokenKind::sReg;
@@ -419,9 +422,6 @@ std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() {
419422
if (Params->Visibility.has_value())
420423
Sampler.Visibility = Params->Visibility.value();
421424

422-
if (consumeExpectedToken(TokenKind::pu_r_paren))
423-
return std::nullopt;
424-
425425
return Sampler;
426426
}
427427

@@ -682,9 +682,9 @@ RootSignatureParser::parseStaticSamplerParams() {
682682
"Expects to only be invoked starting at given token");
683683

684684
ParsedStaticSamplerParams Params;
685-
do {
686-
// `s` POS_INT
685+
while (!peekExpectedToken(TokenKind::pu_r_paren)) {
687686
if (tryConsumeExpectedToken(TokenKind::sReg)) {
687+
// `s` POS_INT
688688
if (Params.Reg.has_value()) {
689689
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
690690
return std::nullopt;
@@ -693,10 +693,8 @@ RootSignatureParser::parseStaticSamplerParams() {
693693
if (!Reg.has_value())
694694
return std::nullopt;
695695
Params.Reg = Reg;
696-
}
697-
698-
// `filter` `=` FILTER
699-
if (tryConsumeExpectedToken(TokenKind::kw_filter)) {
696+
} else if (tryConsumeExpectedToken(TokenKind::kw_filter)) {
697+
// `filter` `=` FILTER
700698
if (Params.Filter.has_value()) {
701699
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
702700
return std::nullopt;
@@ -709,10 +707,8 @@ RootSignatureParser::parseStaticSamplerParams() {
709707
if (!Filter.has_value())
710708
return std::nullopt;
711709
Params.Filter = Filter;
712-
}
713-
714-
// `addressU` `=` TEXTURE_ADDRESS
715-
if (tryConsumeExpectedToken(TokenKind::kw_addressU)) {
710+
} else if (tryConsumeExpectedToken(TokenKind::kw_addressU)) {
711+
// `addressU` `=` TEXTURE_ADDRESS
716712
if (Params.AddressU.has_value()) {
717713
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
718714
return std::nullopt;
@@ -725,10 +721,8 @@ RootSignatureParser::parseStaticSamplerParams() {
725721
if (!AddressU.has_value())
726722
return std::nullopt;
727723
Params.AddressU = AddressU;
728-
}
729-
730-
// `addressV` `=` TEXTURE_ADDRESS
731-
if (tryConsumeExpectedToken(TokenKind::kw_addressV)) {
724+
} else if (tryConsumeExpectedToken(TokenKind::kw_addressV)) {
725+
// `addressV` `=` TEXTURE_ADDRESS
732726
if (Params.AddressV.has_value()) {
733727
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
734728
return std::nullopt;
@@ -741,10 +735,8 @@ RootSignatureParser::parseStaticSamplerParams() {
741735
if (!AddressV.has_value())
742736
return std::nullopt;
743737
Params.AddressV = AddressV;
744-
}
745-
746-
// `addressW` `=` TEXTURE_ADDRESS
747-
if (tryConsumeExpectedToken(TokenKind::kw_addressW)) {
738+
} else if (tryConsumeExpectedToken(TokenKind::kw_addressW)) {
739+
// `addressW` `=` TEXTURE_ADDRESS
748740
if (Params.AddressW.has_value()) {
749741
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
750742
return std::nullopt;
@@ -757,10 +749,8 @@ RootSignatureParser::parseStaticSamplerParams() {
757749
if (!AddressW.has_value())
758750
return std::nullopt;
759751
Params.AddressW = AddressW;
760-
}
761-
762-
// `mipLODBias` `=` NUMBER
763-
if (tryConsumeExpectedToken(TokenKind::kw_mipLODBias)) {
752+
} else if (tryConsumeExpectedToken(TokenKind::kw_mipLODBias)) {
753+
// `mipLODBias` `=` NUMBER
764754
if (Params.MipLODBias.has_value()) {
765755
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
766756
return std::nullopt;
@@ -773,10 +763,8 @@ RootSignatureParser::parseStaticSamplerParams() {
773763
if (!MipLODBias.has_value())
774764
return std::nullopt;
775765
Params.MipLODBias = MipLODBias;
776-
}
777-
778-
// `maxAnisotropy` `=` POS_INT
779-
if (tryConsumeExpectedToken(TokenKind::kw_maxAnisotropy)) {
766+
} else if (tryConsumeExpectedToken(TokenKind::kw_maxAnisotropy)) {
767+
// `maxAnisotropy` `=` POS_INT
780768
if (Params.MaxAnisotropy.has_value()) {
781769
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
782770
return std::nullopt;
@@ -789,10 +777,8 @@ RootSignatureParser::parseStaticSamplerParams() {
789777
if (!MaxAnisotropy.has_value())
790778
return std::nullopt;
791779
Params.MaxAnisotropy = MaxAnisotropy;
792-
}
793-
794-
// `comparisonFunc` `=` COMPARISON_FUNC
795-
if (tryConsumeExpectedToken(TokenKind::kw_comparisonFunc)) {
780+
} else if (tryConsumeExpectedToken(TokenKind::kw_comparisonFunc)) {
781+
// `comparisonFunc` `=` COMPARISON_FUNC
796782
if (Params.CompFunc.has_value()) {
797783
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
798784
return std::nullopt;
@@ -805,10 +791,8 @@ RootSignatureParser::parseStaticSamplerParams() {
805791
if (!CompFunc.has_value())
806792
return std::nullopt;
807793
Params.CompFunc = CompFunc;
808-
}
809-
810-
// `borderColor` `=` STATIC_BORDER_COLOR
811-
if (tryConsumeExpectedToken(TokenKind::kw_borderColor)) {
794+
} else if (tryConsumeExpectedToken(TokenKind::kw_borderColor)) {
795+
// `borderColor` `=` STATIC_BORDER_COLOR
812796
if (Params.BorderColor.has_value()) {
813797
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
814798
return std::nullopt;
@@ -821,10 +805,8 @@ RootSignatureParser::parseStaticSamplerParams() {
821805
if (!BorderColor.has_value())
822806
return std::nullopt;
823807
Params.BorderColor = BorderColor;
824-
}
825-
826-
// `minLOD` `=` NUMBER
827-
if (tryConsumeExpectedToken(TokenKind::kw_minLOD)) {
808+
} else if (tryConsumeExpectedToken(TokenKind::kw_minLOD)) {
809+
// `minLOD` `=` NUMBER
828810
if (Params.MinLOD.has_value()) {
829811
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
830812
return std::nullopt;
@@ -837,10 +819,8 @@ RootSignatureParser::parseStaticSamplerParams() {
837819
if (!MinLOD.has_value())
838820
return std::nullopt;
839821
Params.MinLOD = MinLOD;
840-
}
841-
842-
// `maxLOD` `=` NUMBER
843-
if (tryConsumeExpectedToken(TokenKind::kw_maxLOD)) {
822+
} else if (tryConsumeExpectedToken(TokenKind::kw_maxLOD)) {
823+
// `maxLOD` `=` NUMBER
844824
if (Params.MaxLOD.has_value()) {
845825
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
846826
return std::nullopt;
@@ -853,10 +833,8 @@ RootSignatureParser::parseStaticSamplerParams() {
853833
if (!MaxLOD.has_value())
854834
return std::nullopt;
855835
Params.MaxLOD = MaxLOD;
856-
}
857-
858-
// `space` `=` POS_INT
859-
if (tryConsumeExpectedToken(TokenKind::kw_space)) {
836+
} else if (tryConsumeExpectedToken(TokenKind::kw_space)) {
837+
// `space` `=` POS_INT
860838
if (Params.Space.has_value()) {
861839
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
862840
return std::nullopt;
@@ -869,10 +847,8 @@ RootSignatureParser::parseStaticSamplerParams() {
869847
if (!Space.has_value())
870848
return std::nullopt;
871849
Params.Space = Space;
872-
}
873-
874-
// `visibility` `=` SHADER_VISIBILITY
875-
if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
850+
} else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
851+
// `visibility` `=` SHADER_VISIBILITY
876852
if (Params.Visibility.has_value()) {
877853
reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
878854
return std::nullopt;
@@ -885,8 +861,17 @@ RootSignatureParser::parseStaticSamplerParams() {
885861
if (!Visibility.has_value())
886862
return std::nullopt;
887863
Params.Visibility = Visibility;
864+
} else {
865+
consumeNextToken(); // position to start of invalid token
866+
reportDiag(diag::err_hlsl_rootsig_invalid_param)
867+
<< /*param of=*/TokenKind::kw_StaticSampler;
868+
return std::nullopt;
888869
}
889-
} while (tryConsumeExpectedToken(TokenKind::pu_comma));
870+
871+
// ',' denotes another element, otherwise, expected to be at ')'
872+
if (!tryConsumeExpectedToken(TokenKind::pu_comma))
873+
break;
874+
}
890875

891876
return Params;
892877
}

‎clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1374,4 +1374,31 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidDescriptorClauseParamsCommaTest) {
13741374
ASSERT_TRUE(Consumer->isSatisfied());
13751375
}
13761376

1377+
TEST_F(ParseHLSLRootSignatureTest, InvalidStaticSamplerCommaTest) {
1378+
// This test will check that an error is produced when there is a missing
1379+
// comma between parameters
1380+
const llvm::StringLiteral Source = R"cc(
1381+
StaticSampler(
1382+
s0
1383+
maxLOD = 3
1384+
)
1385+
)cc";
1386+
1387+
auto Ctx = createMinimalASTContext();
1388+
StringLiteral *Signature = wrapSource(Ctx, Source);
1389+
1390+
TrivialModuleLoader ModLoader;
1391+
auto PP = createPP(Source, ModLoader);
1392+
1393+
SmallVector<RootSignatureElement> Elements;
1394+
hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements,
1395+
Signature, *PP);
1396+
1397+
// Test correct diagnostic produced
1398+
Consumer->setExpected(diag::err_expected);
1399+
ASSERT_TRUE(Parser.parse());
1400+
1401+
ASSERT_TRUE(Consumer->isSatisfied());
1402+
}
1403+
13771404
} // anonymous namespace

0 commit comments

Comments
 (0)