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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits