[clang] Update argument checking tablegen code to use a 'full' name #99993
Conversation
In 92fc1eb the HLSLLoopHint attribute was added with an 'unroll' spelling. There is an existing LoopHint attribute with the same spelling. These attributes have different arguments. The tablegen used to produce checks on arguments uses only the attribute name, making it impossible to return correct info for attribute with different argument types but the same name. Improve the situation by using a 'full' name that combines the syntax, scope, and name. This allows, for example, #pragma unroll and [[unroll(x)]] to coexist correctly even with different argument types. Also fix a bug in the StrictEnumParameters tablegen. If will now correctly specify each parameter instead of only the first.
llvmbot
added
clang
|
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-clang Author: Mike Rice (mikerice1969) ChangesIn 92fc1eb the HLSLLoopHint attribute was added with an 'unroll' spelling. There is an existing LoopHint attribute with the same spelling. These attributes have different arguments. The tablegen used to produce checks on arguments uses only the attribute name, making it impossible to return correct info for attribute with different argument types but the same name. Improve the situation by using a 'full' name that combines the syntax, scope, and name. This allows, for example, #pragma unroll and [[unroll(x)]] to coexist correctly even with different argument types. Also fix a bug in the StrictEnumParameters tablegen. If will now correctly specify each parameter instead of only the first. Patch is 31.28 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/99993.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/AttributeCommonInfo.h b/clang/include/clang/Basic/AttributeCommonInfo.h
index 5f024b4b5fd78..2c5baaf921cdf 100644
--- a/clang/include/clang/Basic/AttributeCommonInfo.h
+++ b/clang/include/clang/Basic/AttributeCommonInfo.h
@@ -191,6 +191,12 @@ class AttributeCommonInfo {
/// __gnu__::__attr__ will be normalized to gnu::attr).
std::string getNormalizedFullName() const;
+ /// Gets a normalized full name, with syntax, scope and name.
+ static std::string
+ getNormalizedFullNameWithSyntax(const IdentifierInfo *Name,
+ const IdentifierInfo *Scope,
+ AttributeCommonInfo::Syntax SyntaxUsed);
+
bool isDeclspecAttribute() const { return SyntaxUsed == AS_Declspec; }
bool isMicrosoftAttribute() const { return SyntaxUsed == AS_Microsoft; }
diff --git a/clang/lib/Basic/Attributes.cpp b/clang/lib/Basic/Attributes.cpp
index 867d241a2cf84..0625c7637c009 100644
--- a/clang/lib/Basic/Attributes.cpp
+++ b/clang/lib/Basic/Attributes.cpp
@@ -153,6 +153,40 @@ std::string AttributeCommonInfo::getNormalizedFullName() const {
normalizeName(getAttrName(), getScopeName(), getSyntax()));
}
+static StringRef getSyntaxName(AttributeCommonInfo::Syntax SyntaxUsed) {
+ switch (SyntaxUsed) {
+ case AttributeCommonInfo::AS_GNU:
+ return "GNU";
+ case AttributeCommonInfo::AS_CXX11:
+ return "CXX11";
+ case AttributeCommonInfo::AS_C23:
+ return "C23";
+ case AttributeCommonInfo::AS_Declspec:
+ return "Declspec";
+ case AttributeCommonInfo::AS_Microsoft:
+ return "Microsoft";
+ case AttributeCommonInfo::AS_Keyword:
+ return "Keyword";
+ case AttributeCommonInfo::AS_Pragma:
+ return "Pragma";
+ case AttributeCommonInfo::AS_ContextSensitiveKeyword:
+ return "ContextSensitiveKeyword";
+ case AttributeCommonInfo::AS_HLSLAnnotation:
+ return "HLSLAnnotation";
+ case AttributeCommonInfo::AS_Implicit:
+ return "Implicit";
+ }
+}
+
+std::string AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ const IdentifierInfo *Name, const IdentifierInfo *ScopeName,
+ Syntax SyntaxUsed) {
+ std::string FullName = getSyntaxName(SyntaxUsed).str();
+ FullName += "::";
+ return FullName +=
+ static_cast<std::string>(normalizeName(Name, ScopeName, SyntaxUsed));
+}
+
unsigned AttributeCommonInfo::calculateAttributeSpellingListIndex() const {
// Both variables will be used in tablegen generated
// attribute spell list index matching code.
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7ce9a9cea1c7a..28c69f9981abd 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -314,9 +314,13 @@ void Parser::ParseGNUAttributes(ParsedAttributes &Attrs,
}
/// Determine whether the given attribute has an identifier argument.
-static bool attributeHasIdentifierArg(const IdentifierInfo &II) {
+static bool attributeHasIdentifierArg(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_IDENTIFIER_ARG_LIST
- return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<bool>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(false);
#undef CLANG_ATTR_IDENTIFIER_ARG_LIST
@@ -324,54 +328,78 @@ static bool attributeHasIdentifierArg(const IdentifierInfo &II) {
/// Determine whether the given attribute has an identifier argument.
static ParsedAttributeArgumentsProperties
-attributeStringLiteralListArg(const llvm::Triple &T, const IdentifierInfo &II) {
+attributeStringLiteralListArg(const llvm::Triple &T, const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_STRING_LITERAL_ARG_LIST
- return llvm::StringSwitch<uint32_t>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<uint32_t>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(0);
#undef CLANG_ATTR_STRING_LITERAL_ARG_LIST
}
/// Determine whether the given attribute has a variadic identifier argument.
-static bool attributeHasVariadicIdentifierArg(const IdentifierInfo &II) {
+static bool attributeHasVariadicIdentifierArg(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_VARIADIC_IDENTIFIER_ARG_LIST
- return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<bool>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(false);
#undef CLANG_ATTR_VARIADIC_IDENTIFIER_ARG_LIST
}
/// Determine whether the given attribute treats kw_this as an identifier.
-static bool attributeTreatsKeywordThisAsIdentifier(const IdentifierInfo &II) {
+static bool attributeTreatsKeywordThisAsIdentifier(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST
- return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<bool>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(false);
#undef CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST
}
/// Determine if an attribute accepts parameter packs.
-static bool attributeAcceptsExprPack(const IdentifierInfo &II) {
+static bool attributeAcceptsExprPack(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_ACCEPTS_EXPR_PACK
- return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<bool>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(false);
#undef CLANG_ATTR_ACCEPTS_EXPR_PACK
}
/// Determine whether the given attribute parses a type argument.
-static bool attributeIsTypeArgAttr(const IdentifierInfo &II) {
+static bool attributeIsTypeArgAttr(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_TYPE_ARG_LIST
- return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<bool>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(false);
#undef CLANG_ATTR_TYPE_ARG_LIST
}
/// Determine whether the given attribute takes identifier arguments.
-static bool attributeHasStrictIdentifierArgs(const IdentifierInfo &II) {
+static bool attributeHasStrictIdentifierArgs(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_STRICT_IDENTIFIER_ARG_AT_INDEX_LIST
- return (llvm::StringSwitch<uint64_t>(normalizeAttrName(II.getName()))
+ return (llvm::StringSwitch<uint64_t>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(0)) != 0;
#undef CLANG_ATTR_STRICT_IDENTIFIER_ARG_AT_INDEX_LIST
@@ -380,9 +408,13 @@ static bool attributeHasStrictIdentifierArgs(const IdentifierInfo &II) {
/// Determine whether the given attribute takes an identifier argument at a
/// specific index
static bool attributeHasStrictIdentifierArgAtIndex(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName,
size_t argIndex) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_STRICT_IDENTIFIER_ARG_AT_INDEX_LIST
- return (llvm::StringSwitch<uint64_t>(normalizeAttrName(II.getName()))
+ return (llvm::StringSwitch<uint64_t>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(0)) &
(1ull << argIndex);
@@ -391,9 +423,13 @@ static bool attributeHasStrictIdentifierArgAtIndex(const IdentifierInfo &II,
/// Determine whether the given attribute requires parsing its arguments
/// in an unevaluated context or not.
-static bool attributeParsedArgsUnevaluated(const IdentifierInfo &II) {
+static bool attributeParsedArgsUnevaluated(const IdentifierInfo &II,
+ ParsedAttr::Syntax Syntax,
+ IdentifierInfo *ScopeName) {
+ std::string FullName = AttributeCommonInfo::getNormalizedFullNameWithSyntax(
+ &II, ScopeName, Syntax);
#define CLANG_ATTR_ARG_CONTEXT_LIST
- return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<bool>(FullName)
#include "clang/Parse/AttrParserStringSwitches.inc"
.Default(false);
#undef CLANG_ATTR_ARG_CONTEXT_LIST
@@ -523,10 +559,12 @@ unsigned Parser::ParseAttributeArgsCommon(
// Ignore the left paren location for now.
ConsumeParen();
- bool ChangeKWThisToIdent = attributeTreatsKeywordThisAsIdentifier(*AttrName);
- bool AttributeIsTypeArgAttr = attributeIsTypeArgAttr(*AttrName);
+ bool ChangeKWThisToIdent = attributeTreatsKeywordThisAsIdentifier(
+ *AttrName, Form.getSyntax(), ScopeName);
+ bool AttributeIsTypeArgAttr =
+ attributeIsTypeArgAttr(*AttrName, Form.getSyntax(), ScopeName);
bool AttributeHasVariadicIdentifierArg =
- attributeHasVariadicIdentifierArg(*AttrName);
+ attributeHasVariadicIdentifierArg(*AttrName, Form.getSyntax(), ScopeName);
// Interpret "kw_this" as an identifier if the attributed requests it.
if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
@@ -535,8 +573,9 @@ unsigned Parser::ParseAttributeArgsCommon(
ArgsVector ArgExprs;
if (Tok.is(tok::identifier)) {
// If this attribute wants an 'identifier' argument, make it so.
- bool IsIdentifierArg = AttributeHasVariadicIdentifierArg ||
- attributeHasIdentifierArg(*AttrName);
+ bool IsIdentifierArg =
+ AttributeHasVariadicIdentifierArg ||
+ attributeHasIdentifierArg(*AttrName, Form.getSyntax(), ScopeName);
ParsedAttr::Kind AttrKind =
ParsedAttr::getParsedKind(AttrName, ScopeName, Form.getSyntax());
@@ -568,7 +607,8 @@ unsigned Parser::ParseAttributeArgsCommon(
if (T.isUsable())
TheParsedType = T.get();
} else if (AttributeHasVariadicIdentifierArg ||
- attributeHasStrictIdentifierArgs(*AttrName)) {
+ attributeHasStrictIdentifierArgs(*AttrName, Form.getSyntax(),
+ ScopeName)) {
// Parse variadic identifier arg. This can either consume identifiers or
// expressions. Variadic identifier args do not support parameter packs
// because those are typically used for attributes with enumeration
@@ -579,8 +619,9 @@ unsigned Parser::ParseAttributeArgsCommon(
if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
Tok.setKind(tok::identifier);
- if (Tok.is(tok::identifier) && attributeHasStrictIdentifierArgAtIndex(
- *AttrName, ArgExprs.size())) {
+ if (Tok.is(tok::identifier) &&
+ attributeHasStrictIdentifierArgAtIndex(
+ *AttrName, Form.getSyntax(), ScopeName, ArgExprs.size())) {
ArgExprs.push_back(ParseIdentifierLoc());
continue;
}
@@ -589,7 +630,8 @@ unsigned Parser::ParseAttributeArgsCommon(
if (Tok.is(tok::identifier)) {
ArgExprs.push_back(ParseIdentifierLoc());
} else {
- bool Uneval = attributeParsedArgsUnevaluated(*AttrName);
+ bool Uneval = attributeParsedArgsUnevaluated(
+ *AttrName, Form.getSyntax(), ScopeName);
EnterExpressionEvaluationContext Unevaluated(
Actions,
Uneval ? Sema::ExpressionEvaluationContext::Unevaluated
@@ -610,7 +652,8 @@ unsigned Parser::ParseAttributeArgsCommon(
} while (TryConsumeToken(tok::comma));
} else {
// General case. Parse all available expressions.
- bool Uneval = attributeParsedArgsUnevaluated(*AttrName);
+ bool Uneval = attributeParsedArgsUnevaluated(*AttrName, Form.getSyntax(),
+ ScopeName);
EnterExpressionEvaluationContext Unevaluated(
Actions,
Uneval ? Sema::ExpressionEvaluationContext::Unevaluated
@@ -621,7 +664,8 @@ unsigned Parser::ParseAttributeArgsCommon(
ExprVector ParsedExprs;
ParsedAttributeArgumentsProperties ArgProperties =
- attributeStringLiteralListArg(getTargetInfo().getTriple(), *AttrName);
+ attributeStringLiteralListArg(getTargetInfo().getTriple(), *AttrName,
+ Form.getSyntax(), ScopeName);
if (ParseAttributeArgumentList(*AttrName, ParsedExprs, ArgProperties)) {
SkipUntil(tok::r_paren, StopAtSemi);
return 0;
@@ -632,7 +676,7 @@ unsigned Parser::ParseAttributeArgsCommon(
if (!isa<PackExpansionExpr>(ParsedExprs[I]))
continue;
- if (!attributeAcceptsExprPack(*AttrName)) {
+ if (!attributeAcceptsExprPack(*AttrName, Form.getSyntax(), ScopeName)) {
Diag(Tok.getLocation(),
diag::err_attribute_argument_parm_pack_not_supported)
<< AttrName;
@@ -696,7 +740,7 @@ void Parser::ParseGNUAttributeArgs(
ParseTypeTagForDatatypeAttribute(*AttrName, AttrNameLoc, Attrs, EndLoc,
ScopeName, ScopeLoc, Form);
return;
- } else if (attributeIsTypeArgAttr(*AttrName)) {
+ } else if (attributeIsTypeArgAttr(*AttrName, Form.getSyntax(), ScopeName)) {
ParseAttributeWithTypeArg(*AttrName, AttrNameLoc, Attrs, ScopeName,
ScopeLoc, Form);
return;
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 7f452d177c16f..bc15504e87487 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -594,13 +594,6 @@ static Attr *handleHLSLLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
unsigned UnrollFactor = 0;
if (A.getNumArgs() == 1) {
-
- if (A.isArgIdent(0)) {
- S.Diag(A.getLoc(), diag::err_attribute_argument_type)
- << A << AANT_ArgumentIntegerConstant << A.getRange();
- return nullptr;
- }
-
Expr *E = A.getArgAsExpr(0);
if (S.CheckLoopHintExpr(E, St->getBeginLoc(),
diff --git a/clang/test/SemaHLSL/Loops/unroll.hlsl b/clang/test/SemaHLSL/Loops/unroll.hlsl
index 2e2be319e4666..c94dc5857c2e8 100644
--- a/clang/test/SemaHLSL/Loops/unroll.hlsl
+++ b/clang/test/SemaHLSL/Loops/unroll.hlsl
@@ -1,7 +1,10 @@
// RUN: %clang_cc1 -O0 -finclude-default-header -fsyntax-only -triple dxil-pc-shadermodel6.6-library %s -verify
void unroll_no_vars() {
+ // expected-note@+1 {{declared here}}
int I = 3;
- [unroll(I)] // expected-error {{'unroll' attribute requires an integer constant}}
+ // expected-error@+2 {{expression is not an integral constant expression}}
+ // expected-note@+1 {{read of non-const variable 'I' is not allowed in a constant expression}}
+ [unroll(I)]
while (I--);
}
diff --git a/clang/test/TableGen/attrs-parser-string-switches.td b/clang/test/TableGen/attrs-parser-string-switches.td
new file mode 100644
index 0000000000000..c15ab104e0ccd
--- /dev/null
+++ b/clang/test/TableGen/attrs-parser-string-switches.td
@@ -0,0 +1,232 @@
+// RUN: clang-tblgen -gen-clang-attr-parser-string-switches -I%p/../../include %s -o - 2>&1 | FileCheck %s
+
+// Tests that the tablegen can support attributes with the same spellings but
+// different argument types.
+
+include "clang/Basic/Attr.td"
+
+// Test attributeParsedArgsUnevaluated : different ParseArgumentsAsUnevaluated
+def TestUnEvalOne : InheritableAttr {
+ let Spellings = [Clang<"test_uneval">];
+ let Args = [ExprArgument<"Count">];
+ let Subjects = SubjectList<[Function]>;
+ let ParseArgumentsAsUnevaluated = 1;
+ let Documentation = [Undocumented];
+}
+
+def TestUnEvalTwo : InheritableAttr {
+ let Spellings = [Pragma<"", "test_uneval">];
+ let Args = [ExprArgument<"Count">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
+}
+
+// CHECK: #if defined(CLANG_ATTR_ARG_CONTEXT_LIST)
+// CHECK-NOT: .Case("Pragma::test_uneval", true)
+// CHECK: .Case("GNU::test_uneval", true)
+// CHECK-NOT: .Case("Pragma::test_uneval", true)
+// CHECK: .Case("CXX11::clang::test_uneval", true)
+// CHECK-NOT: .Case("Pragma::test_uneval", true)
+// CHECK: .Case("C23::clang::test_uneval", true)
+// CHECK-NOT: .Case("Pragma::test_uneval", true)
+// CHECK: #endif // CLANG_ATTR_ARG_CONTEXT_LIST
+
+// Test attributeHasIdentifierArg: Same spelling, one with and one without
+// an IdentifierArg.
+def TestIdentOne : Attr {
+ let Spellings = [Clang<"test_ident">];
+ let Args = [EnumArgument<"Option", "OptionType", /*is_string=*/false,
+ ["optA", "optB"], ["OPTA", "OPTB"]>];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
+}
+
+def TestIdentTwo : StmtAttr {
+ let Spellings = [Pragma<"", "test_ident">];
+ let Args = [UnsignedArgument<"val", /*opt*/1>];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
+}
+
+// CHECK: #if defined(CLANG_ATTR_IDENTIFIER_ARG_LIST)
+// CHECK-NOT: .Case("Pragma::test_ident", true)
+// CHECK: .Case("GNU::test_ident", true)
+// CHECK-NOT: .Case("Pragma::test_ident", true)
+// CHECK: .Case("CXX11::clang::test_ident", true)
+// CHECK-NOT: .Case("Pragma::test_ident", true)
+// CHECK: .Case("C23::clang::test_ident", true)
+// CHECK-NOT: .Case("Pragma::test_ident", true)
+// CHECK: #endif // CLANG_ATTR_IDENTIFIER_ARG_LIST
+
+// Test attributeStringLiteralListArg : Same spelling, some with a
+// StringArgument, some without, some in different locations.
+def TestStringOne : DeclOrTypeAttr {
+ let Spellings = [Clang<"test_string">];
+ let Args = [StringArgument<"strarg">];
+ let Subjects = SubjectList<[Function, TypedefName, ParmVar]>;
+ let Documentation = [AcquireHandleDocs];
+}
+
+def TestStringTwo : InheritableAttr {
+ let Spellings = [Pragma<"", "test_string">];
+ let Args = [UnsignedArgument<"unsarg">];
+ let Subjects = SubjectList<[Function], ErrorDiag>;
+ let Documentation = [Undocumented];
+}
+
+// In a different position
+def TestStringThree : Attr {
+ let Spellings = [Declspec<"test_string">];
+ let Args = [UnsignedArgument<"uarg">, StringArgument<"strarg">];
+ let Subjects = SubjectList<[Function, TypedefName, ParmVar]>;
+ let Documentation = [AcquireHandleDocs];
+}
+
+// CHECK: #if defined(CLANG_ATTR_STRING_LITERAL_ARG_LIST)
+// CHECK-NOT: .Case("Pragma::test_string"
+// CHECK: .Case("GNU::test_string", 1)
+// CHECK: .Case("CXX11::clang::test_string", 1)
+// CHECK: .Case("C23::clang::test_string", 1)
+// CHECK-NOT: .Case("Pragma::test_string"
+// CHECK: .Case("Declspec::test_string", 2)
+// CHECK-NOT: .Case("Pragma::test_string"
+// CHECK: #endif // CLANG_ATTR_STRING_LITERAL_ARG_LIST
+
+// Test attributeHasVariadicIdentifierArg : One with VariadicIdentifierArgument
+// and one without.
+def TestVariadicIdentOne : InheritableAttr {
+ let Spellings = [Clang<"test_var_ident">];
+ let Args = [VariadicIdentifierArgument<"iargs">];
+ let Subject...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Also fix a bug in the StrictEnumParameters tablegen. If will now correctly specify each parameter instead of only the first.
Does this impact anything user-facing? e.g., should there be an additional test somewhere in clang/test/Sema/ for this change?
Precommit CI failures seem unrelated.
clang/lib/Basic/Attributes.cpp
Outdated
| std::string FullName = getSyntaxName(SyntaxUsed).str(); | ||
| FullName += "::"; | ||
| return FullName += | ||
| static_cast<std::string>(normalizeName(Name, ScopeName, SyntaxUsed)); | ||
| } |
There was a problem hiding this comment.
This seems like a place to use llvm::Twine() before converting to a std::string.
I don't think there is any user-visible issue with the one attribute that uses this. At least how the Parse code is written currently. The attribute has three EnumArguments followed by an IntArgument. The call to attributeHasStrictIdentifierArgAtIndex is only done if the token is an identifier and if the IntArgument is an identifier that same thing happens anyway. Looking at this code now, I am not convinced the call to attributeHasStrictIdentifierArgAtIndex does anything useful. So I think we can remove the whole function attributeHasStrictIdentifierArgAtIndex and the related tablegen. No tests fail without it. The function attributeHasStrictIdentifierArgs still seems necessary but not the AtIndex. What do you think? |
Thanks!
That code was added very recently by #94056 and seem to be specific to the |
@AaronBallman The code is logically useless so there's no way to add tests that make it useful. It doesn't matter what attributeHasStrictIdentifierArgAtIndex returns we will always just add identifiers to ArgExprs. Unless @ojhunt and/or @ahmedbougacha can clarify I think we should just remove it. |
I agree that the code is useless and should be removed, but what concerns me is that it's a fairly significant chunk of code that certainly wasn't written by accident, so I don't know if #94056 should be pulled back entirely or not. I'll bring this up on the other PR however so we can unblock this one. |
|
@AaronBallman hmmm, looks like we may have lost some diff at some point, I'll investigate to work out what is going on |
|
@AaronBallman ah with more context, I see we didn't actually land "// Do something if not an identifier" (implying partial commit history at some point". I'm looking at exactly what happens, or if we're missing some test cases -- it's possible that when written there was the intent to either add more similar parameters, do better error checks, or possibly just over engineering. Will need to investigate further to verify, will try to determine the expected behaviour today or tomorrow (I agree if it is not necessary it should be removed - ie. if it was just added "for the future" or similar and it's had quite a few years for that future to happen and not be needed) |
|
This causes significant compile-time regressions, especially for unoptimized builds: https://llvm-compile-time-tracker.com/compare.php?from=39b6900852e7a1187bd742ba5c1387ca1be58e2c&to=2acf77f987331c05520c5bfd849326909ffce983&stat=instructions:u You probably need to separately match the syntax and the name without constructing a temporary |
@nikic Thanks. I'll look into improving this. If anyone thinks this should be reverted until then let me know, or just go ahead and revert it. |
|
The old tablegen produced a single entry per name:
The new tablegen can produce multiple entries: I think we can instead produce something like this, one per name: That should elimate the need for any new std::strings. @AaronBallman does this sound reasonable? |
Thank you for identifying this, @nikic!
That seems like a good approach to me |
|
Proposed fix for compile-time regression #101768 |
In 92fc1eb the HLSLLoopHint attribute was added with an 'unroll' spelling. There is an existing LoopHint attribute with the same spelling. These attributes have different arguments.
The tablegen used to produce checks on arguments uses only the attribute name, making it impossible to return correct info for attribute with different argument types but the same name.
Improve the situation by using a 'full' name that combines the syntax, scope, and name. This allows, for example, #pragma unroll and [[unroll(x)]] to coexist correctly even with different argument types.
Also fix a bug in the StrictEnumParameters tablegen. If will now correctly specify each parameter instead of only the first.