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

Reply via email to