[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Committed (with additional comments on deviating attributes) in r320752.


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

alexfh wrote:
> aaron.ballman wrote:
> > dcoughlin wrote:
> > > aaron.ballman wrote:
> > > > dcoughlin wrote:
> > > > > aaron.ballman wrote:
> > > > > > rsmith wrote:
> > > > > > > Hmm, should the clang static analyzer reuse the `clang::` 
> > > > > > > namespace, or should it get its own?
> > > > > > Good question, I don't have strong opinions on the answer here, but 
> > > > > > perhaps @dcoughlin does?
> > > > > > 
> > > > > > If we want to use a separate namespace for the analyzer, would we 
> > > > > > want to use that same namespace for any clang-tidy specific 
> > > > > > attributes? Or should clang-tidy get its own namespace? (Do we ever 
> > > > > > plan to execute clang-tidy through the clang driver? That might 
> > > > > > change our answer.)
> > > > > How would this look if we added a special namespace for the clang 
> > > > > static analyzer? Would this lead to duplication (say, 
> > > > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the 
> > > > > "analyzer_" prefix for __attribute__((analyzer_noreturn))? Or could 
> > > > > we have the "analyzer_" prefix only for GNU-style attributes but not 
> > > > > for C++ (for example, [[clang_analyzer::noreturn]])?
> > > > > 
> > > > > As for clang-tidy, I think it probably makes sense for it to have its 
> > > > > own namespace, but we should ask @alexfh.
> > > > > How would this look if we added a special namespace for the clang 
> > > > > static analyzer? Would this lead to duplication (say, 
> > > > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the 
> > > > > "analyzer_" prefix for attribute((analyzer_noreturn))? Or could we 
> > > > > have the "analyzer_" prefix only for GNU-style attributes but not for 
> > > > > C++ (for example, [[clang_analyzer::noreturn]])?
> > > > 
> > > > We have the ability to do whatever we'd like there. Given that the 
> > > > semantics are so similar to `[[noreturn]]`, I think it would be 
> > > > reasonable to use `[[clang_analyzer::noreturn]]` and 
> > > > `__attribute__((analyzer_noreturn))` if that's the direction you think 
> > > > is best.
> > > > 
> > > > > As for clang-tidy, I think it probably makes sense for it to have its 
> > > > > own namespace, but we should ask @alexfh.
> > > > 
> > > > I'm less enthusiastic about giving clang-tidy a vendor namespace that's 
> > > > separate from the static analyzer, should the need arise. My biggest 
> > > > concern there is that I would *really* like to see clang-tidy be more 
> > > > tightly integrated with the clang driver (so users don't have to 
> > > > manually execute a secondary tool). If that were to happen, then the 
> > > > user experience would be that there are two vendor namespaces both 
> > > > related to analyzer attributes.
> > > > 
> > > > That said, I would also not be opposed to putting all of these 
> > > > attributes under the `clang` vendor namespace and not having a separate 
> > > > vendor for the analyzer or clang-tidy.
> > > I would be find with keeping all of these things under the `clang` 
> > > namespace, too.
> > > 
> > > That said, I do think there is some value in having a namespace for 
> > > analyzer attributes separate from clang proper because the namespace 
> > > would make it more clear that the attribute doesn't affect code 
> > > generation.
> > I've changed this one back to the GNU spelling to give us time to decide 
> > how we want to handle analyzer attributes.
> > 
> > I'm not certain "does not affect codegen" is the correct measure to use for 
> > this, however. We have other attributes that muddy the water, such as 
> > `annotate`, or the format specifier attributes -- these don't (really) 
> > impact codegen in any way, but do impact more than just the analyzer. Given 
> > the integration of the analyzer with Clang (and the somewhat fluid nature 
> > of what is responsible for issuing diagnostics), I think we should proceed 
> > with caution on the idea of an analyzer-specific namespace.
> > 
> > However, do you have a list of attributes you think might qualify as being 
> > analyzer-only? I can make sure we leave those spellings alone in this patch.
> An argument against clang_tidy and clang_analyzer vendor namespaces is that 
> the choice of where to implement a certain check would be connected to adding 
> an attribute in one or both of these namespaces, which would complicate 
> things a bit. In case both clang-tidy and static analyzer use the same 
> argument, we'd need to have duplicate attributes. I definitely don't think we 
> need three `noreturn` attributes, for example.
Yeah, that's basically the concern I was getting at -- it really ties our hands 
on where the various checks live in a syntactic 

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

aaron.ballman wrote:
> dcoughlin wrote:
> > aaron.ballman wrote:
> > > dcoughlin wrote:
> > > > aaron.ballman wrote:
> > > > > rsmith wrote:
> > > > > > Hmm, should the clang static analyzer reuse the `clang::` 
> > > > > > namespace, or should it get its own?
> > > > > Good question, I don't have strong opinions on the answer here, but 
> > > > > perhaps @dcoughlin does?
> > > > > 
> > > > > If we want to use a separate namespace for the analyzer, would we 
> > > > > want to use that same namespace for any clang-tidy specific 
> > > > > attributes? Or should clang-tidy get its own namespace? (Do we ever 
> > > > > plan to execute clang-tidy through the clang driver? That might 
> > > > > change our answer.)
> > > > How would this look if we added a special namespace for the clang 
> > > > static analyzer? Would this lead to duplication (say, 
> > > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > > > prefix for __attribute__((analyzer_noreturn))? Or could we have the 
> > > > "analyzer_" prefix only for GNU-style attributes but not for C++ (for 
> > > > example, [[clang_analyzer::noreturn]])?
> > > > 
> > > > As for clang-tidy, I think it probably makes sense for it to have its 
> > > > own namespace, but we should ask @alexfh.
> > > > How would this look if we added a special namespace for the clang 
> > > > static analyzer? Would this lead to duplication (say, 
> > > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > > > prefix for attribute((analyzer_noreturn))? Or could we have the 
> > > > "analyzer_" prefix only for GNU-style attributes but not for C++ (for 
> > > > example, [[clang_analyzer::noreturn]])?
> > > 
> > > We have the ability to do whatever we'd like there. Given that the 
> > > semantics are so similar to `[[noreturn]]`, I think it would be 
> > > reasonable to use `[[clang_analyzer::noreturn]]` and 
> > > `__attribute__((analyzer_noreturn))` if that's the direction you think is 
> > > best.
> > > 
> > > > As for clang-tidy, I think it probably makes sense for it to have its 
> > > > own namespace, but we should ask @alexfh.
> > > 
> > > I'm less enthusiastic about giving clang-tidy a vendor namespace that's 
> > > separate from the static analyzer, should the need arise. My biggest 
> > > concern there is that I would *really* like to see clang-tidy be more 
> > > tightly integrated with the clang driver (so users don't have to manually 
> > > execute a secondary tool). If that were to happen, then the user 
> > > experience would be that there are two vendor namespaces both related to 
> > > analyzer attributes.
> > > 
> > > That said, I would also not be opposed to putting all of these attributes 
> > > under the `clang` vendor namespace and not having a separate vendor for 
> > > the analyzer or clang-tidy.
> > I would be find with keeping all of these things under the `clang` 
> > namespace, too.
> > 
> > That said, I do think there is some value in having a namespace for 
> > analyzer attributes separate from clang proper because the namespace would 
> > make it more clear that the attribute doesn't affect code generation.
> I've changed this one back to the GNU spelling to give us time to decide how 
> we want to handle analyzer attributes.
> 
> I'm not certain "does not affect codegen" is the correct measure to use for 
> this, however. We have other attributes that muddy the water, such as 
> `annotate`, or the format specifier attributes -- these don't (really) impact 
> codegen in any way, but do impact more than just the analyzer. Given the 
> integration of the analyzer with Clang (and the somewhat fluid nature of what 
> is responsible for issuing diagnostics), I think we should proceed with 
> caution on the idea of an analyzer-specific namespace.
> 
> However, do you have a list of attributes you think might qualify as being 
> analyzer-only? I can make sure we leave those spellings alone in this patch.
An argument against clang_tidy and clang_analyzer vendor namespaces is that the 
choice of where to implement a certain check would be connected to adding an 
attribute in one or both of these namespaces, which would complicate things a 
bit. In case both clang-tidy and static analyzer use the same argument, we'd 
need to have duplicate attributes. I definitely don't think we need three 
`noreturn` attributes, for example.


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM.

I think the `LateParsed` attributes are not going to work properly (because 
they'd be written in places where the function parameters are not in scope), so 
I wonder if we should avoid allowing them with [[clang::]] notation for now so 
we can choose to make them function type attributes in the future, but I'm 
happy with you making that call.


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 126015.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Updated based on review feedback. This patch leaves off attributes with custom 
parsing, as well as `analyzer_noreturn`, so that we can handle them as special 
cases.


https://reviews.llvm.org/D40625

Files:
  include/clang/Basic/Attr.td

Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -513,7 +513,7 @@
 }
 
 def AddressSpace : TypeAttr {
-  let Spellings = [GNU<"address_space">];
+  let Spellings = [Clang<"address_space">];
   let Args = [IntArgument<"AddressSpace">];
   let Documentation = [Undocumented];
 }
@@ -598,7 +598,7 @@
 }
 
 def Annotate : InheritableParamAttr {
-  let Spellings = [GNU<"annotate">];
+  let Spellings = [Clang<"annotate">];
   let Args = [StringArgument<"Annotation">];
   // Ensure that the annotate attribute can be used with
   // '#pragma clang attribute' even though it has no subject list.
@@ -700,7 +700,7 @@
 }
 
 def Blocks : InheritableAttr {
-  let Spellings = [GNU<"blocks">];
+  let Spellings = [Clang<"blocks">];
   let Args = [EnumArgument<"Type", "BlockType", ["byref"], ["ByRef"]>];
   let Documentation = [Undocumented];
 }
@@ -727,34 +727,34 @@
 // cf_returns_retained attributes.  It is generally applied by
 // '#pragma clang arc_cf_code_audited' rather than explicitly.
 def CFAuditedTransfer : InheritableAttr {
-  let Spellings = [GNU<"cf_audited_transfer">];
+  let Spellings = [Clang<"cf_audited_transfer">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 // cf_unknown_transfer is an explicit opt-out of cf_audited_transfer.
 // It indicates that the function has unknown or unautomatable
 // transfer semantics.
 def CFUnknownTransfer : InheritableAttr {
-  let Spellings = [GNU<"cf_unknown_transfer">];
+  let Spellings = [Clang<"cf_unknown_transfer">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 def CFReturnsRetained : InheritableAttr {
-  let Spellings = [GNU<"cf_returns_retained">];
+  let Spellings = [Clang<"cf_returns_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
   let Documentation = [Undocumented];
 }
 
 def CFReturnsNotRetained : InheritableAttr {
-  let Spellings = [GNU<"cf_returns_not_retained">];
+  let Spellings = [Clang<"cf_returns_not_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
   let Documentation = [Undocumented];
 }
 
 def CFConsumed : InheritableParamAttr {
-  let Spellings = [GNU<"cf_consumed">];
+  let Spellings = [Clang<"cf_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
   let Documentation = [Undocumented];
 }
@@ -875,7 +875,7 @@
 }
 
 def CXX11NoReturn : InheritableAttr {
-  let Spellings = [CXX11<"","noreturn", 200809>];
+  let Spellings = [CXX11<"", "noreturn", 200809>];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [CXX11NoReturnDocs];
 }
@@ -1030,13 +1030,13 @@
 }
 
 def MinSize : InheritableAttr {
-  let Spellings = [GNU<"minsize">];
+  let Spellings = [Clang<"minsize">];
   let Subjects = SubjectList<[Function, ObjCMethod], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 def FlagEnum : InheritableAttr {
-  let Spellings = [GNU<"flag_enum">];
+  let Spellings = [Clang<"flag_enum">];
   let Subjects = SubjectList<[Enum]>;
   let Documentation = [FlagEnumDocs];
 }
@@ -1085,22 +1085,22 @@
 }
 
 def IBAction : InheritableAttr {
-  let Spellings = [GNU<"ibaction">];
+  let Spellings = [Clang<"ibaction">];
   let Subjects = SubjectList<[ObjCInstanceMethod]>;
   // An AST node is created for this attribute, but is not used by other parts
   // of the compiler. However, this node needs to exist in the AST because
   // external tools rely on it.
   let Documentation = [Undocumented];
 }
 
 def IBOutlet : InheritableAttr {
-  let Spellings = [GNU<"iboutlet">];
+  let Spellings = [Clang<"iboutlet">];
 //  let Subjects = [ObjCIvar, ObjCProperty];
   let Documentation = [Undocumented];
 }
 
 def IBOutletCollection : InheritableAttr {
-  let Spellings = [GNU<"iboutletcollection">];
+  let Spellings = [Clang<"iboutletcollection">];
   let Args = [TypeArgument<"Interface", 1>];
 //  let Subjects = [ObjCIvar, ObjCProperty];
   let Documentation = [Undocumented];
@@ -1210,13 +1210,13 @@
 }
 
 def NeonPolyVectorType : TypeAttr {
-  let Spellings = [GNU<"neon_polyvector_type">];
+  let Spellings = [Clang<"neon_polyvector_type">];
   let Args = [IntArgument<"NumElements">];
   let Documentation = [Undocumented];
 }
 
 def NeonVectorType : TypeAttr {
-  let Spellings = [GNU<"neon_vector_type">];
+  let Spellings = [Clang<"neon_vector_type">];
   let Args = [IntArgument<"NumElements">];
   let Documentation = [Undocumented];
 }
@@ -1299,28 +1299,28 @@
 // this should be rejected on 

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 4 inline comments as done.
aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:1218-1228
 def NeonPolyVectorType : TypeAttr {
-  let Spellings = [GNU<"neon_polyvector_type">];
+  let Spellings = [Clang<"neon_polyvector_type">];
   let Args = [IntArgument<"NumElements">];
   let Documentation = [Undocumented];
 }
 
 def NeonVectorType : TypeAttr {

sbaranga wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > I *think* these are a Clang invention rather than part of the ARM NEON 
> > > > intrinsics specification, but perhaps you could ask someone from ARM to 
> > > > confirm that.
> > > @sbaranga or @jmolloy -- do you happen to know the answer to this, or 
> > > know someone who does?
> > Pinging this comment.
> Yes, these should be internal to clang and used to implement the ACLE 
> specification (which defines the NEON intrinsics). The ACLE doesn't define 
> these.
> 
> Here is a link to the latest spec: 
> https://developer.arm.com/products/software-development-tools/compilers/arm-compiler-5/docs/101028/latest/1-preface
Perfect, thank you @sbaranga!


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[silviu.bara...@arm.com via Phabricator via cfe-commits]

sbaranga added inline comments.



Comment at: include/clang/Basic/Attr.td:1218-1228
 def NeonPolyVectorType : TypeAttr {
-  let Spellings = [GNU<"neon_polyvector_type">];
+  let Spellings = [Clang<"neon_polyvector_type">];
   let Args = [IntArgument<"NumElements">];
   let Documentation = [Undocumented];
 }
 
 def NeonVectorType : TypeAttr {

aaron.ballman wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > I *think* these are a Clang invention rather than part of the ARM NEON 
> > > intrinsics specification, but perhaps you could ask someone from ARM to 
> > > confirm that.
> > @sbaranga or @jmolloy -- do you happen to know the answer to this, or know 
> > someone who does?
> Pinging this comment.
Yes, these should be internal to clang and used to implement the ACLE 
specification (which defines the NEON intrinsics). The ACLE doesn't define 
these.

Here is a link to the latest spec: 
https://developer.arm.com/products/software-development-tools/compilers/arm-compiler-5/docs/101028/latest/1-preface


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

dcoughlin wrote:
> aaron.ballman wrote:
> > dcoughlin wrote:
> > > aaron.ballman wrote:
> > > > rsmith wrote:
> > > > > Hmm, should the clang static analyzer reuse the `clang::` namespace, 
> > > > > or should it get its own?
> > > > Good question, I don't have strong opinions on the answer here, but 
> > > > perhaps @dcoughlin does?
> > > > 
> > > > If we want to use a separate namespace for the analyzer, would we want 
> > > > to use that same namespace for any clang-tidy specific attributes? Or 
> > > > should clang-tidy get its own namespace? (Do we ever plan to execute 
> > > > clang-tidy through the clang driver? That might change our answer.)
> > > How would this look if we added a special namespace for the clang static 
> > > analyzer? Would this lead to duplication (say, 
> > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > > prefix for __attribute__((analyzer_noreturn))? Or could we have the 
> > > "analyzer_" prefix only for GNU-style attributes but not for C++ (for 
> > > example, [[clang_analyzer::noreturn]])?
> > > 
> > > As for clang-tidy, I think it probably makes sense for it to have its own 
> > > namespace, but we should ask @alexfh.
> > > How would this look if we added a special namespace for the clang static 
> > > analyzer? Would this lead to duplication (say, 
> > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > > prefix for attribute((analyzer_noreturn))? Or could we have the 
> > > "analyzer_" prefix only for GNU-style attributes but not for C++ (for 
> > > example, [[clang_analyzer::noreturn]])?
> > 
> > We have the ability to do whatever we'd like there. Given that the 
> > semantics are so similar to `[[noreturn]]`, I think it would be reasonable 
> > to use `[[clang_analyzer::noreturn]]` and 
> > `__attribute__((analyzer_noreturn))` if that's the direction you think is 
> > best.
> > 
> > > As for clang-tidy, I think it probably makes sense for it to have its own 
> > > namespace, but we should ask @alexfh.
> > 
> > I'm less enthusiastic about giving clang-tidy a vendor namespace that's 
> > separate from the static analyzer, should the need arise. My biggest 
> > concern there is that I would *really* like to see clang-tidy be more 
> > tightly integrated with the clang driver (so users don't have to manually 
> > execute a secondary tool). If that were to happen, then the user experience 
> > would be that there are two vendor namespaces both related to analyzer 
> > attributes.
> > 
> > That said, I would also not be opposed to putting all of these attributes 
> > under the `clang` vendor namespace and not having a separate vendor for the 
> > analyzer or clang-tidy.
> I would be find with keeping all of these things under the `clang` namespace, 
> too.
> 
> That said, I do think there is some value in having a namespace for analyzer 
> attributes separate from clang proper because the namespace would make it 
> more clear that the attribute doesn't affect code generation.
I've changed this one back to the GNU spelling to give us time to decide how we 
want to handle analyzer attributes.

I'm not certain "does not affect codegen" is the correct measure to use for 
this, however. We have other attributes that muddy the water, such as 
`annotate`, or the format specifier attributes -- these don't (really) impact 
codegen in any way, but do impact more than just the analyzer. Given the 
integration of the analyzer with Clang (and the somewhat fluid nature of what 
is responsible for issuing diagnostics), I think we should proceed with caution 
on the idea of an analyzer-specific namespace.

However, do you have a list of attributes you think might qualify as being 
analyzer-only? I can make sure we leave those spellings alone in this patch.



Comment at: include/clang/Basic/Attr.td:649
 def Availability : InheritableAttr {
-  let Spellings = [GNU<"availability">];
+  let Spellings = [Clang<"availability">];
   let Args = [IdentifierArgument<"platform">, VersionArgument<"introduced">,

aaron.ballman wrote:
> rsmith wrote:
> > Does the custom parsing for this work for the C++11 attribute syntax?
> Nope; I'll do this one separately (and make sure no other attributes 
> similarly use custom parsing).
I've rolled back the following attributes with custom parsing to only use the 
GNU spelling for right now: availability, objc_bridge_related, 
argument_with_type_tag, pointer_with_type_tag, type_tag_for_datatype.



Comment at: include/clang/Basic/Attr.td:1218-1228
 def NeonPolyVectorType : TypeAttr {
-  let Spellings = [GNU<"neon_polyvector_type">];
+  let Spellings = 

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

aaron.ballman wrote:
> dcoughlin wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > Hmm, should the clang static analyzer reuse the `clang::` namespace, or 
> > > > should it get its own?
> > > Good question, I don't have strong opinions on the answer here, but 
> > > perhaps @dcoughlin does?
> > > 
> > > If we want to use a separate namespace for the analyzer, would we want to 
> > > use that same namespace for any clang-tidy specific attributes? Or should 
> > > clang-tidy get its own namespace? (Do we ever plan to execute clang-tidy 
> > > through the clang driver? That might change our answer.)
> > How would this look if we added a special namespace for the clang static 
> > analyzer? Would this lead to duplication (say, 
> > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > prefix for __attribute__((analyzer_noreturn))? Or could we have the 
> > "analyzer_" prefix only for GNU-style attributes but not for C++ (for 
> > example, [[clang_analyzer::noreturn]])?
> > 
> > As for clang-tidy, I think it probably makes sense for it to have its own 
> > namespace, but we should ask @alexfh.
> > How would this look if we added a special namespace for the clang static 
> > analyzer? Would this lead to duplication (say, 
> > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > prefix for attribute((analyzer_noreturn))? Or could we have the "analyzer_" 
> > prefix only for GNU-style attributes but not for C++ (for example, 
> > [[clang_analyzer::noreturn]])?
> 
> We have the ability to do whatever we'd like there. Given that the semantics 
> are so similar to `[[noreturn]]`, I think it would be reasonable to use 
> `[[clang_analyzer::noreturn]]` and `__attribute__((analyzer_noreturn))` if 
> that's the direction you think is best.
> 
> > As for clang-tidy, I think it probably makes sense for it to have its own 
> > namespace, but we should ask @alexfh.
> 
> I'm less enthusiastic about giving clang-tidy a vendor namespace that's 
> separate from the static analyzer, should the need arise. My biggest concern 
> there is that I would *really* like to see clang-tidy be more tightly 
> integrated with the clang driver (so users don't have to manually execute a 
> secondary tool). If that were to happen, then the user experience would be 
> that there are two vendor namespaces both related to analyzer attributes.
> 
> That said, I would also not be opposed to putting all of these attributes 
> under the `clang` vendor namespace and not having a separate vendor for the 
> analyzer or clang-tidy.
I would be find with keeping all of these things under the `clang` namespace, 
too.

That said, I do think there is some value in having a namespace for analyzer 
attributes separate from clang proper because the namespace would make it more 
clear that the attribute doesn't affect code generation.


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

dcoughlin wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > Hmm, should the clang static analyzer reuse the `clang::` namespace, or 
> > > should it get its own?
> > Good question, I don't have strong opinions on the answer here, but perhaps 
> > @dcoughlin does?
> > 
> > If we want to use a separate namespace for the analyzer, would we want to 
> > use that same namespace for any clang-tidy specific attributes? Or should 
> > clang-tidy get its own namespace? (Do we ever plan to execute clang-tidy 
> > through the clang driver? That might change our answer.)
> How would this look if we added a special namespace for the clang static 
> analyzer? Would this lead to duplication (say, 
> [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" prefix 
> for __attribute__((analyzer_noreturn))? Or could we have the "analyzer_" 
> prefix only for GNU-style attributes but not for C++ (for example, 
> [[clang_analyzer::noreturn]])?
> 
> As for clang-tidy, I think it probably makes sense for it to have its own 
> namespace, but we should ask @alexfh.
> How would this look if we added a special namespace for the clang static 
> analyzer? Would this lead to duplication (say, 
> [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" prefix 
> for attribute((analyzer_noreturn))? Or could we have the "analyzer_" prefix 
> only for GNU-style attributes but not for C++ (for example, 
> [[clang_analyzer::noreturn]])?

We have the ability to do whatever we'd like there. Given that the semantics 
are so similar to `[[noreturn]]`, I think it would be reasonable to use 
`[[clang_analyzer::noreturn]]` and `__attribute__((analyzer_noreturn))` if 
that's the direction you think is best.

> As for clang-tidy, I think it probably makes sense for it to have its own 
> namespace, but we should ask @alexfh.

I'm less enthusiastic about giving clang-tidy a vendor namespace that's 
separate from the static analyzer, should the need arise. My biggest concern 
there is that I would *really* like to see clang-tidy be more tightly 
integrated with the clang driver (so users don't have to manually execute a 
secondary tool). If that were to happen, then the user experience would be that 
there are two vendor namespaces both related to analyzer attributes.

That said, I would also not be opposed to putting all of these attributes under 
the `clang` vendor namespace and not having a separate vendor for the analyzer 
or clang-tidy.


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a subscriber: alexfh.
dcoughlin added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

aaron.ballman wrote:
> rsmith wrote:
> > Hmm, should the clang static analyzer reuse the `clang::` namespace, or 
> > should it get its own?
> Good question, I don't have strong opinions on the answer here, but perhaps 
> @dcoughlin does?
> 
> If we want to use a separate namespace for the analyzer, would we want to use 
> that same namespace for any clang-tidy specific attributes? Or should 
> clang-tidy get its own namespace? (Do we ever plan to execute clang-tidy 
> through the clang driver? That might change our answer.)
How would this look if we added a special namespace for the clang static 
analyzer? Would this lead to duplication (say, 
[[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" prefix 
for __attribute__((analyzer_noreturn))? Or could we have the "analyzer_" prefix 
only for GNU-style attributes but not for C++ (for example, 
[[clang_analyzer::noreturn]])?

As for clang-tidy, I think it probably makes sense for it to have its own 
namespace, but we should ask @alexfh.


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: dcoughlin, sbaranga, jmolloy.
aaron.ballman added a comment.

Added @dcoughlin for opinions about the static analyzer, added @sbaranga and 
@jmolloy for questions about NEON.




Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

rsmith wrote:
> Hmm, should the clang static analyzer reuse the `clang::` namespace, or 
> should it get its own?
Good question, I don't have strong opinions on the answer here, but perhaps 
@dcoughlin does?

If we want to use a separate namespace for the analyzer, would we want to use 
that same namespace for any clang-tidy specific attributes? Or should 
clang-tidy get its own namespace? (Do we ever plan to execute clang-tidy 
through the clang driver? That might change our answer.)



Comment at: include/clang/Basic/Attr.td:649
 def Availability : InheritableAttr {
-  let Spellings = [GNU<"availability">];
+  let Spellings = [Clang<"availability">];
   let Args = [IdentifierArgument<"platform">, VersionArgument<"introduced">,

rsmith wrote:
> Does the custom parsing for this work for the C++11 attribute syntax?
Nope; I'll do this one separately (and make sure no other attributes similarly 
use custom parsing).



Comment at: include/clang/Basic/Attr.td:1218-1228
 def NeonPolyVectorType : TypeAttr {
-  let Spellings = [GNU<"neon_polyvector_type">];
+  let Spellings = [Clang<"neon_polyvector_type">];
   let Args = [IntArgument<"NumElements">];
   let Documentation = [Undocumented];
 }
 
 def NeonVectorType : TypeAttr {

rsmith wrote:
> I *think* these are a Clang invention rather than part of the ARM NEON 
> intrinsics specification, but perhaps you could ask someone from ARM to 
> confirm that.
@sbaranga or @jmolloy -- do you happen to know the answer to this, or know 
someone who does?


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

Hmm, should the clang static analyzer reuse the `clang::` namespace, or should 
it get its own?



Comment at: include/clang/Basic/Attr.td:649
 def Availability : InheritableAttr {
-  let Spellings = [GNU<"availability">];
+  let Spellings = [Clang<"availability">];
   let Args = [IdentifierArgument<"platform">, VersionArgument<"introduced">,

Does the custom parsing for this work for the C++11 attribute syntax?



Comment at: include/clang/Basic/Attr.td:1218-1228
 def NeonPolyVectorType : TypeAttr {
-  let Spellings = [GNU<"neon_polyvector_type">];
+  let Spellings = [Clang<"neon_polyvector_type">];
   let Args = [IntArgument<"NumElements">];
   let Documentation = [Undocumented];
 }
 
 def NeonVectorType : TypeAttr {

I *think* these are a Clang invention rather than part of the ARM NEON 
intrinsics specification, but perhaps you could ask someone from ARM to confirm 
that.


https://reviews.llvm.org/D40625



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
Herald added subscribers: kristof.beyls, Anastasia, mehdi_amini, aemerson.

Based on discussions at the WG21 meeting in Albuquerque and follow-up email 
discussions, I believe the correct approach to exposing attributes from Clang 
is to provide them with GNU-style __attribute__ spellings as well as C++-style 
[[]] spellings in the clang vendor namespace, when appropriate. If the 
attribute was originally provided by a different vendor, and Clang intends to 
be compatible with those semantics, then the attribute should only be provided 
with the spellings supported by the original vendor. Otherwise, any attributes 
provided only by Clang should be exposed as both a GNU-style and C++-style 
attribute as appropriate for the semantics of that attribute.

To that end, this patch adds a C++ spelling in the clang vendor namespace to a 
number of attributes. Similarly, it adds a GNU spelling to the 
`lto_visibility_public` attribute (which was the only one with a C++ spelling 
but not a GNU spelling).

Finally, it leaves the following attributes with only a GNU spelling, based on 
the given rationale:

align_value -- originally provided by Intel with only a GNU spelling

constant, cudart_builtin, device, device_builtin_surface_type, 
device_builtin_texture_type, host, launch_bounds, shared, nv_weak -- attributes 
specified by CUDA with only a GNU spelling (several are also ignored attributes)

opencl_unroll_hint, intel_reqd_sub_group_size, nosvm, ext_vector_type, 
reqd_work_group_size, work_group_size_hint, vec_type_hint -- attributes 
specified by OpenCL with only a GNU spelling

kernel -- specified by RenderScript

carries_dependency -- supported as a standards-based attribute, shouldn't be in 
a vendor namespace

enable_if, diagnose_if, guarded_by, pt_guarded_by, acquired_after, 
acquired_before, assert_exclusive_lock, assert_shared_lock, 
exclusive_trylock_function, shared_trylock_function, lock_returned, 
locks_excluded -- not easily used with the C++ spelling because the attribute 
requires naming function parameters (these might be good candidates to explore 
changing into type attributes in the future).

bounded -- currently ignored

*Please double-check my understanding of these attribute spellings.* It is 
possible that I've misunderstood attributes as being specified by other sources 
when in fact they are Clang extensions, or that something is listed as a Clang 
extension but is actually a compatibility attribute. If my understanding is 
correct, I'll add the rationales to Attr.td as comments alongside the attribute 
spelling so that we don't lose that information.

Because this is a mechanical change that only introduces new spellings, there 
are no proposed test cases for the patch.


https://reviews.llvm.org/D40625

Files:
  include/clang/Basic/Attr.td

Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -518,7 +518,7 @@
 }
 
 def AddressSpace : TypeAttr {
-  let Spellings = [GNU<"address_space">];
+  let Spellings = [Clang<"address_space">];
   let Args = [IntArgument<"AddressSpace">];
   let Documentation = [Undocumented];
 }
@@ -599,12 +599,12 @@
 }
 
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];
 }
 
 def Annotate : InheritableParamAttr {
-  let Spellings = [GNU<"annotate">];
+  let Spellings = [Clang<"annotate">];
   let Args = [StringArgument<"Annotation">];
   // Ensure that the annotate attribute can be used with
   // '#pragma clang attribute' even though it has no subject list.
@@ -646,7 +646,7 @@
 }
 
 def Availability : InheritableAttr {
-  let Spellings = [GNU<"availability">];
+  let Spellings = [Clang<"availability">];
   let Args = [IdentifierArgument<"platform">, VersionArgument<"introduced">,
   VersionArgument<"deprecated">, VersionArgument<"obsoleted">,
   BoolArgument<"unavailable">, StringArgument<"message">,
@@ -706,7 +706,7 @@
 }
 
 def Blocks : InheritableAttr {
-  let Spellings = [GNU<"blocks">];
+  let Spellings = [Clang<"blocks">];
   let Args = [EnumArgument<"Type", "BlockType", ["byref"], ["ByRef"]>];
   let Documentation = [Undocumented];
 }
@@ -733,34 +733,34 @@
 // cf_returns_retained attributes.  It is generally applied by
 // '#pragma clang arc_cf_code_audited' rather than explicitly.
 def CFAuditedTransfer : InheritableAttr {
-  let Spellings = [GNU<"cf_audited_transfer">];
+  let Spellings = [Clang<"cf_audited_transfer">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 // cf_unknown_transfer is an explicit opt-out of cf_audited_transfer.
 // It indicates that the function has unknown or unautomatable
 // transfer semantics.
 def CFUnknownTransfer : InheritableAttr {
-  let Spellings =