[Clang] Add __ugly__ spelling for the msvc attribute scope #113765
Conversation
You can test this locally with the following command:git-clang-format --diff d906ac52ab8ee46090a6696f4ffb34c40ee6abb7 68f3617027fa09b531b5e3eaea6a025c6783a258 --extensions cpp,h -- clang/include/clang/Basic/Attributes.h clang/lib/Basic/Attributes.cpp clang/lib/Sema/SemaCodeComplete.cpp clang/test/SemaCXX/cxx2a-ms-no-unique-address.cppView the diff from clang-format here.diff --git a/clang/include/clang/Basic/Attributes.h b/clang/include/clang/Basic/Attributes.h
index 9fa7e5ce55..498a4a2e41 100644
--- a/clang/include/clang/Basic/Attributes.h
+++ b/clang/include/clang/Basic/Attributes.h
@@ -23,7 +23,7 @@ int hasAttribute(AttributeCommonInfo::Syntax Syntax,
const IdentifierInfo *Scope, const IdentifierInfo *Attr,
const TargetInfo &Target, const LangOptions &LangOpts);
-inline const char* deuglifyAttrScope(StringRef Scope) {
+inline const char *deuglifyAttrScope(StringRef Scope) {
if (Scope == "_Clang")
return "clang";
if (Scope == "__gnu__")
@@ -33,7 +33,7 @@ inline const char* deuglifyAttrScope(StringRef Scope) {
return nullptr;
}
-inline const char* uglifyAttrScope(StringRef Scope) {
+inline const char *uglifyAttrScope(StringRef Scope) {
if (Scope == "clang")
return "_Clang";
if (Scope == "gnu")
|
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesThis is required to avoid macro clashing when using attributes like This patch also refactor the logic for attribute scope uglification into a single place to make it easier to add additional cases in case another one is required at some point again. Full diff: https://github.com/llvm/llvm-project/pull/113765.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/Attributes.h b/clang/include/clang/Basic/Attributes.h
index 61666a6f4d9ac4..9fa7e5ce55d491 100644
--- a/clang/include/clang/Basic/Attributes.h
+++ b/clang/include/clang/Basic/Attributes.h
@@ -23,6 +23,30 @@ int hasAttribute(AttributeCommonInfo::Syntax Syntax,
const IdentifierInfo *Scope, const IdentifierInfo *Attr,
const TargetInfo &Target, const LangOptions &LangOpts);
+inline const char* deuglifyAttrScope(StringRef Scope) {
+ if (Scope == "_Clang")
+ return "clang";
+ if (Scope == "__gnu__")
+ return "gnu";
+ if (Scope == "__msvc__")
+ return "msvc";
+ return nullptr;
+}
+
+inline const char* uglifyAttrScope(StringRef Scope) {
+ if (Scope == "clang")
+ return "_Clang";
+ if (Scope == "gnu")
+ return "__gnu__";
+ if (Scope == "msvc")
+ return "__msvc__";
+ return nullptr;
+}
+
+inline bool isPotentiallyUglyScope(StringRef Scope) {
+ return Scope == "gnu" || Scope == "clang" || Scope == "msvc";
+}
+
} // end namespace clang
#endif // LLVM_CLANG_BASIC_ATTRIBUTES_H
diff --git a/clang/lib/Basic/Attributes.cpp b/clang/lib/Basic/Attributes.cpp
index 867d241a2cf847..4afa129e3b2222 100644
--- a/clang/lib/Basic/Attributes.cpp
+++ b/clang/lib/Basic/Attributes.cpp
@@ -38,10 +38,8 @@ int clang::hasAttribute(AttributeCommonInfo::Syntax Syntax,
// Normalize the scope name, but only for gnu and clang attributes.
StringRef ScopeName = Scope ? Scope->getName() : "";
- if (ScopeName == "__gnu__")
- ScopeName = "gnu";
- else if (ScopeName == "_Clang")
- ScopeName = "clang";
+ if (const char *prettyName = deuglifyAttrScope(ScopeName))
+ ScopeName = prettyName;
// As a special case, look for the omp::sequence and omp::directive
// attributes. We support those, but not through the typical attribute
@@ -87,10 +85,8 @@ normalizeAttrScopeName(const IdentifierInfo *Scope,
StringRef ScopeName = Scope->getName();
if (SyntaxUsed == AttributeCommonInfo::AS_CXX11 ||
SyntaxUsed == AttributeCommonInfo::AS_C23) {
- if (ScopeName == "__gnu__")
- ScopeName = "gnu";
- else if (ScopeName == "_Clang")
- ScopeName = "clang";
+ if (const char *prettySpelling = deuglifyAttrScope(ScopeName))
+ return prettySpelling;
}
return ScopeName;
}
@@ -100,12 +96,11 @@ static StringRef normalizeAttrName(const IdentifierInfo *Name,
AttributeCommonInfo::Syntax SyntaxUsed) {
// Normalize the attribute name, __foo__ becomes foo. This is only allowable
// for GNU attributes, and attributes using the double square bracket syntax.
- bool ShouldNormalize =
- SyntaxUsed == AttributeCommonInfo::AS_GNU ||
- ((SyntaxUsed == AttributeCommonInfo::AS_CXX11 ||
- SyntaxUsed == AttributeCommonInfo::AS_C23) &&
- (NormalizedScopeName.empty() || NormalizedScopeName == "gnu" ||
- NormalizedScopeName == "clang"));
+ bool ShouldNormalize = SyntaxUsed == AttributeCommonInfo::AS_GNU ||
+ ((SyntaxUsed == AttributeCommonInfo::AS_CXX11 ||
+ SyntaxUsed == AttributeCommonInfo::AS_C23) &&
+ (NormalizedScopeName.empty() ||
+ isPotentiallyUglyScope(NormalizedScopeName)));
StringRef AttrName = Name->getName();
if (ShouldNormalize && AttrName.size() >= 4 && AttrName.starts_with("__") &&
AttrName.ends_with("__"))
diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp
index 3e31f3d82657a3..732da9ceb0628f 100644
--- a/clang/lib/Sema/SemaCodeComplete.cpp
+++ b/clang/lib/Sema/SemaCodeComplete.cpp
@@ -25,6 +25,7 @@
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Type.h"
#include "clang/Basic/AttributeCommonInfo.h"
+#include "clang/Basic/Attributes.h"
#include "clang/Basic/CharInfo.h"
#include "clang/Basic/OperatorKinds.h"
#include "clang/Basic/Specifiers.h"
@@ -4579,22 +4580,6 @@ void SemaCodeCompletion::CodeCompleteDeclSpec(Scope *S, DeclSpec &DS,
Results.size());
}
-static const char *underscoreAttrScope(llvm::StringRef Scope) {
- if (Scope == "clang")
- return "_Clang";
- if (Scope == "gnu")
- return "__gnu__";
- return nullptr;
-}
-
-static const char *noUnderscoreAttrScope(llvm::StringRef Scope) {
- if (Scope == "_Clang")
- return "clang";
- if (Scope == "__gnu__")
- return "gnu";
- return nullptr;
-}
-
void SemaCodeCompletion::CodeCompleteAttribute(
AttributeCommonInfo::Syntax Syntax, AttributeCompletion Completion,
const IdentifierInfo *InScope) {
@@ -4618,7 +4603,7 @@ void SemaCodeCompletion::CodeCompleteAttribute(
bool InScopeUnderscore = false;
if (InScope) {
InScopeName = InScope->getName();
- if (const char *NoUnderscore = noUnderscoreAttrScope(InScopeName)) {
+ if (const char *NoUnderscore = deuglifyAttrScope(InScopeName)) {
InScopeName = NoUnderscore;
InScopeUnderscore = true;
}
@@ -4653,7 +4638,7 @@ void SemaCodeCompletion::CodeCompleteAttribute(
Results.AddResult(
CodeCompletionResult(Results.getAllocator().CopyString(Scope)));
// Include alternate form (__gnu__ instead of gnu).
- if (const char *Scope2 = underscoreAttrScope(Scope))
+ if (const char *Scope2 = uglifyAttrScope(Scope))
Results.AddResult(CodeCompletionResult(Scope2));
}
continue;
@@ -4712,7 +4697,7 @@ void SemaCodeCompletion::CodeCompleteAttribute(
if (Scope.empty()) {
Add(Scope, Name, /*Underscores=*/true);
} else {
- const char *GuardedScope = underscoreAttrScope(Scope);
+ const char *GuardedScope = uglifyAttrScope(Scope);
if (!GuardedScope)
continue;
Add(GuardedScope, Name, /*Underscores=*/true);
diff --git a/clang/test/SemaCXX/cxx2a-ms-no-unique-address.cpp b/clang/test/SemaCXX/cxx2a-ms-no-unique-address.cpp
index 822ed752fa9c75..c9cf86f5270034 100644
--- a/clang/test/SemaCXX/cxx2a-ms-no-unique-address.cpp
+++ b/clang/test/SemaCXX/cxx2a-ms-no-unique-address.cpp
@@ -16,6 +16,9 @@ struct [[msvc::no_unique_address]] S { // expected-error {{only applies to non-b
[[msvc::no_unique_address()]] int arglist; // expected-error {{cannot have an argument list}} unsupported-warning {{unknown}}
int [[msvc::no_unique_address]] c; // expected-error {{cannot be applied to types}} unsupported-error {{cannot be applied to types}}
+ [[__msvc__::__no_unique_address__]] int d; // unsupported-warning {{unknown}}
+ [[__msvc__::no_unique_address]] int e; // unsupported-warning {{unknown}}
+ [[msvc::__no_unique_address__]] int g; // unsupported-warning {{unknown}}
};
struct CStructNoUniqueAddress {
|
|
How would that be used? MSVC seems to not like the ugly spelling https://compiler-explorer.com/z/Kb85YY18P |
We need |
|
Yeah, it would be great if C1XX (MSVC's frontend) supported I've occasionally gone into the FE and just implemented features; this one might be within my reach. |
|
In principle, I'm not opposed, but if cl.exe doesn't support the syntax for their vendor string, I don't know that we should support it ourselves. The signal of support from @StephanTLavavej is definitely a point in favor of going this route, but I'd feel most comfortable if we were following cl's lead rather than going off on our own. |
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
My only hold up is that we don't really own the design space. If @StephanTLavavej /Microsoft can assert us that this is the spelling they are going with, I'm all for it.
| const IdentifierInfo *Scope, const IdentifierInfo *Attr, | ||
| const TargetInfo &Target, const LangOptions &LangOpts); | ||
|
|
||
| inline const char* deuglifyAttrScope(StringRef Scope) { |
There was a problem hiding this comment.
not a huge fan of ugly here as it isn't particularly descriptive, but mostly just on the name of it.
| return nullptr; | ||
| } | ||
|
|
||
| inline bool isPotentiallyUglyScope(StringRef Scope) { |
There was a problem hiding this comment.
isn't this the inverse? These are the not-ugly ones, right?
|
The MSVC FE team hasn't expressed enthusiasm for adding ugly spellings. If I learn more I'll relay that info. |
Thank you for checking! Unfortunately, I think that's a reason for Clang to not support it for the |
What is the alternative? |
Can you get away with not using or something along those lines. |
This steals both |
I'm personally sympathetic, and would be ok with something that we know MSVC won't step on ( |
AFAICT you're only allowed to __uglify__ attributes with no namespace, |
I see that now :/ I managed to mis-remember that, and did the research AFTER posting! |
You can put guards in to only steal
It's an option, but not one I'm thrilled with. The point to vendor namespaces is that the vendor picks them and other vendors can be compatible. Do we want EDG to have to support
That's somewhat more palatable to me, but I'm still wondering whether we're making a mountain out of a mole hill with the vendor prefix: That said, the attribute name itself does seem to be an issue: So yeah, I think I'd be happier exposing |
|
@AaronBallman I'm not super worried about these specific cases, but I'd like to know how to proceed with |
I think there's three options.
As a non-libc++ person, it's easy for me to support (1). :-D I really think Microsoft needs to address this issue on their end. Also as a non-libc++ person, I could live with (2). I'm not keen on (3) because this would mean that Clang has multiple vendor prefixes that mean "Clang" and makes it a bit confusing as to whether the attributes under the new vendor prefix are ours or Microsoft's when conflicts arise. e.g., we start having the risk of Clang-specific behaviors under one vendor spelling and Microsoft-specific behaviors under the other. |
|
Closing for now. I hope MSVC will add it, but I don't really expect it unfortunately. |
6 participants
This is required to avoid macro clashing when using attributes like
[[msvc::no_unique_address]].This patch also refactor the logic for attribute scope __uglification__ into a single place to make it easier to add additional cases in case another one is required at some point again.