[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 fashion, and that seems like it doesn't help our users any. They don't usually care whether something is a clang-tidy check vs analyzer vs compiler proper.


https://reviews.llvm.org/D40625