aaron.ballman added a comment. 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<>`. > 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
