hfinkel added a comment. In https://reviews.llvm.org/D37436#869445, @aaron.ballman wrote:
> In https://reviews.llvm.org/D37436#869350, @hfinkel wrote: > > > In https://reviews.llvm.org/D37436#868333, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D37436#868295, @hfinkel wrote: > > > > > > > In https://reviews.llvm.org/D37436#867965, @aaron.ballman wrote: > > > > > > > > > In https://reviews.llvm.org/D37436#867287, @rsmith wrote: > > > > > > > > > > > If this is just supposed to be an experiment to get feedback on the > > > > > > feature, then I don't think we should be treating it as a > > > > > > different attribute syntax at all. Rather, I think we > > > > > > just want to permit C++11 attributes to be parsed in other > > > > > > language modes. If/when this becomes part of some future C working > > > > > > draft, I think that's the time to have a > > > > > > separate attribute syntax with a distinct set of valid unqualified > > > > > > attribute names. > > > > > > > > > > > > > > > I do not think that's the correct approach. These are not C++ > > > > > attributes (for instance, no `using` insanity is allowed, `::` is a > > > > > new lexing token in C, etc). Also, I don't think it's a good idea to > > > > > enable all C++11-style attributes in C mode without giving each > > > > > attribute some appropriate thought (what does `abi_tag` *do* in C > > > > > mode? What happens with _Noreturn vs [[noreturn]] etc). Also, I'm not > > > > > comfortable adding a bunch of vendor-specific `gnu::` attributes that > > > > > GCC does not implement in C yet. > > > > > > > > > > > > On this last point, I disagree. Implementation experience is about all > > > > of the messy things that occur in production. In production, if we have > > > > this syntax, then we'll end up enabling it for a bunch of > > > > vendor-specific attributes. Do you think that we wouldn't? > > > > > > > > > I'm sure we would. Also, FWIW, I was planning to traverse the attributes > > > we implement to find which clang-specific C++ attributes would make sense > > > to also enable as a follow-up patch once the syntax is in. > > > > > > > N2137 specifically talks about this as a use case. If so, this will > > > > include `gnu::` attributes that we have in Clang (even if GCC does not > > > > implement them). > > > > > > Eventually, yes, but it seems like a problem to implement something under > > > that vendor namespace when the vendor themselves do not. I think it would > > > be really unfortunate were GCC to add a C++ attribute named > > > [[clang::frobble]] that Clang does not implement, and I don't see this > > > case as being all that different. My belief is that GCC will eventually > > > elect to make most of these attributes available in C mode and that's an > > > appropriate time for us to do the same for their vendor namespace. > > > > > > > From my perspective, lack of consistency here between Clang's C and C++ > > > > modes is much more problematic than a lack of consistency between what > > > > Clang and GCC implement. > > > > > > From my perspective, they're both problems in their own right. To me (and > > > maybe I'm weird with this line of reasoning), the only reasonable time to > > > implement an attribute under another vendor's attribute namespace is when > > > you are promising your users that you will attempt to match the owning > > > vendor's semantics for that attribute. A case could be made here that the > > > owning vendor *has* implemented that attribute (since they have in C++), > > > but I'm not too comfortable *assuming* that the GCC folks are okay with > > > this since they don't implement the feature syntax in C yet. > > > > > > That said, I'm happy to ask Jason at the meetings in Albuquerque to > > > explore the idea -- but I don't think it should hold up this patch, > > > especially since we have our own vendor attributes we can use for gaining > > > experience. > > > > > > I certainly understand your perspective, but this is an orthogonal concern. > > If this is something that Clang does, then it should do it consistently. If > > you'd like us not to support `gnu::` attributes that GCC itself does not > > support, and that's something that we currently do in C++, then please > > submit a patch to fix that for all language modes. It should not differ > > between language modes. > > > > Is the problem here that we're treating `gnu::`, not as a vendor prefix, > > but as generic escape hatch to get to anything generally provided via > > GCC-attribute syntax (which many compilers, including ours, have extended > > with attributes that GCC does not itself support)? > > > I definitely agree that we want to be self-consistent, so thank you for > helping me understand where you're coming from. > > I've been very consistent in rejecting patches that add C++ attributes to the > gnu namespace unless GCC also implements them. This most often comes up as a > misunderstanding of when to use the `GNU<>` (just provides support for > `__attribute__(())`) spelling and when to use the GCC<> (provides support for > both `__attribute__(())` and `[[gnu::]]`) spelling. If you know of any > attributes that we've put into the gnu namespace (perhaps through a GCC > spelling) that are not supported by GCC, I'd like to know so that I can fix > them. FWIW, I took a quick look through Attr.td this morning and we have zero > attributes explicitly in the gnu namespace (CXX11<"gnu", "blah">), and I > spot-checked the GCC spellings and did not find any that were not also > documented by GCC. > > We have certainly added GNU-style attributes to Clang that GCC does not > support, and that's totally fine (there is no vendor namespace there which we > could use). I would absolutely welcome discussion as to whether we want to > expose those through a C++11 attribute under the clang namespace (we've done > that in a handful of cases, but have not been consistent about it because > some of those attributes don't apply to C++ code). However, I think that's an > orthogonal patch to this one (and one I would really like to explore once we > have a consistent C and C++ attribute syntax). Basically, I think that > someday we may want to add a `CLANG<>` spelling that exposes the attribute as > `__attribute__(())` and `[[clang::]]` (in both C and C++) and use that > similar to how we handle `GCC<>`. I think that I misunderstood your concern. Let me see if I can summarize your position: You believe that, when GCC implements this syntax in C, they will audit their attributes and not support all of their existing `gnu::` attributes in C. You only want us to support these when we know what that list will be (which we don't yet). Is that correct? > > >> Also, please post a full-context patch. > > Ugh. I swear TortoiseSVN used to handle this properly for me. I'll re-upload > with full context. https://reviews.llvm.org/D37436 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
