This is an archive of the discontinued LLVM Phabricator instance.

Initial implementation of C attributes (WG14 N2137)
ClosedPublic

Authored by aaron.ballman on Sep 4 2017, 6:49 AM.

Details

Summary

WG14 has demonstrated sincere interest in adding C++-style attributes to C for C2x, and have added the proposal to their SD-3 document tracking features which are possibly desired for the next version of the standard. The proposal has *not* been formally adopted into the working draft (C17 needs to be finalized first), but it is at the stage where having implementation experience is important to the committee.

This patch implements the feature as proposed, but it might make sense to split the patch up, depending on what we think is the correct way to proceed. This patch adds c2x as a new language standard as well as -fc-attributes and -fno-c-attributes flags. The intention is that -std=c2x will enable -fc-attributes once the feature is formally adopted by WG14, but -fc-attributes can be used to enable the functionality in earlier standard modes (but is currently required in order to enable the functionality). fno-c-attributes is provided on the chance that the syntax proposed causes parsing issues for Objective-C and users need a way to work around that; however, such cases should be considered bugs that we need to work around similar to Objective-C++ and C++11-style attributes. The patch also adds support for the deprecated attribute (WG14 N2050), mostly so that I would have an attribute handy to test the semantic functionality. Once the basic syntax is committed, I plan to implement the other attributes: nodiscard (WG14 N2051), fallthrough (WG14 N2052), and maybe_unused (WG14 N2053) in separate patches.

Diff Detail

Event Timeline

aaron.ballman created this revision.Sep 4 2017, 6:49 AM
rsmith edited edge metadata.Sep 8 2017, 11:49 AM

Accepting this under -std=c2x is premature. We don't even know whether there will be such a standard yet, and this has not been voted into a working draft. But the -f flag form is OK.

include/clang/Driver/Options.td
607 ↗(On Diff #113747)

Missing underscore between "fc" and "attributes".

lib/Parse/ParseDecl.cpp
1512 ↗(On Diff #113747)

This name should still reference the syntax used, since it is specific to [[...]] attribute syntax. Calling that CXX11 attribute syntax (even when used in C) doesn't seem overly confusing to me. If you want a different name for this, I'd be OK with that, but it should not be simply "Attribute".

aaron.ballman marked 2 inline comments as done.

Updated based on Richard's comments and some further discussion on IRC.

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.

include/clang/Basic/LangOptions.def
140 ↗(On Diff #114447)

Can we give this a more general name, then enable it under CPlusPlus too? That's what we do for other similar features.

include/clang/Driver/Options.td
607 ↗(On Diff #114447)

Hmm. On reflection, if we're going to have a flag to enable C++11 attributes in other language modes, it should probably work in C++98 mode too, so calling this -fc-attributes is probably not the best idea. -fc++11-attributes might make sense, though.

lib/Parse/ParseDecl.cpp
4219 ↗(On Diff #114447)

I don't think we can reasonably say what C2x will do. Also, doesn't this reject valid code such as:

struct __attribute__((deprecated)) Foo;

?

lib/Parse/ParseDeclCXX.cpp
3939–3940 ↗(On Diff #114447)

This change is unnecessary. kw_alignas is not produced by the lexer in modes where we should not parse it.

3947 ↗(On Diff #114447)

[[... in C is not a standards-based attribute list. I'd revert this change too.

3956 ↗(On Diff #114447)

Likewise.

aaron.ballman marked 3 inline comments as done.

Updates based on review feedback.

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.

The goal of this is to get a working implementation of the attribute syntax that WG14 has shown strong support for including in C2x (F: 16, N: 3, A: 1) with this syntax (F: 12, N: 3, A: 4). The experimental part where I need feedback is whether the paper needs to change because of implementation experience. So far, that's not been required. The more we drift from what's proposed in N2137, the less useful the implementation experience will be (esp if we basically turn this into a simple "enable C++ attributes everywhere" flag). I can understand not adding a C2x language mode due to the lack of a working draft, but that lack should not be what drives internal implementation details for the feature -- I think the separation of concerns in this patch is valuable.

include/clang/Basic/LangOptions.def
140 ↗(On Diff #114447)

I'm not keen on naming it 'Attributes', but the other names are worse (GeneralAttributes, StandardAttributes). Do you have a suggestion?

include/clang/Driver/Options.td
607 ↗(On Diff #114447)

I can't think of a reason why you'd want to control C and C++ attributes separately, so I think it makes sense to add a more general name for this.

I'm definitely not keen on that flag name. I wouldn't be opposed to -fattributes, but that may lead to confusion about other vendor-specific attributes (which we could always decide to use flags like -fdeclspec-attributes etc).

What should happen if a user compiles with -std=c++11 -fno-<whatever>-attributes? Do we want to support such a construct?

lib/Parse/ParseDecl.cpp
4219 ↗(On Diff #114447)

Sorry for the lack of context in the patch (TortoiseSVN doesn't make this easy). This has to do with enum specifiers, not struct or unions -- it will reject enum __attribute__((deprecated)) Foo; as not allowing an attribute list *and* as being a forward reference in C++ mode, but accepts in C mode.

lib/Parse/ParseDeclCXX.cpp
3939–3940 ↗(On Diff #114447)

Ah, good to know. I'll remove.

3956 ↗(On Diff #114447)

I'll add a test for this to ensure we don't accidentally allow it in C mode.

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? 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). 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.

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.

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)?

Also, please post a full-context patch.

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.

Added full context, no other changes from previous patch.

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.

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?

Yes, that is correct.

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?

Yes, that is correct.

Okay. A large fraction of the number of attributes we'll want to use are going to fall into this category (because Clang doesn't have its own attributes, but copied GCC's, for many things). I don't think we'll get good implementation feedback until we have this figured out. If we can't sync with GCC soon, I suggest just making a reasonable guess. My first choice would be to just allowing everything, and then we can see what people found to be useful and why. Our experience here can also provide feedback to GCC (and we can always update late if needed - it is still an experimental feature).

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?

Yes, that is correct.

Okay. A large fraction of the number of attributes we'll want to use are going to fall into this category (because Clang doesn't have its own attributes, but copied GCC's, for many things). I don't think we'll get good implementation feedback until we have this figured out. If we can't sync with GCC soon, I suggest just making a reasonable guess. My first choice would be to just allowing everything, and then we can see what people found to be useful and why. Our experience here can also provide feedback to GCC (and we can always update late if needed - it is still an experimental feature).

I think this direction is a reasonable one, but not for the initial syntax patch. I would prefer to be conservative and get the basic syntax in first. My plan is that once the syntax is available, I would start adding more attributes, starting with the ones proposed to WG14, followed by the existing C++ ones in the clang namespace. I think the gnu attributes would likely be immediately following the clang ones, so they're definitely a high priority for me to support.

Btw, because I wasn't explicit about this, we're in violent agreement that getting implementation experience about how well we can share attributes between C and C++ code bases is also an important question to answer.

A large fraction of the number of attributes we'll want to use are going to fall into this category (because Clang doesn't have its own attributes, but copied GCC's, for many things). I don't think we'll get good implementation feedback until we have this figured out. If we can't sync with GCC soon, I suggest just making a reasonable guess. My first choice would be to just allowing everything, and then we can see what people found to be useful and why. Our experience here can also provide feedback to GCC (and we can always update late if needed - it is still an experimental feature).

I think I would be more comfortable taking those attributes that we want to support in C and making them available in the clang:: attribute namespace (across all languages). Because GCC has distinct C and C++ parsers (sharing some, but not all, code) there are often differences between how they handle the same construct in C and C++ mode, and accepting all [[gnu::something]] attributes in C mode seems likely to create incompatibilities, even if GCC decides that all __attribute__s should be available as [[gnu::x]] in C.

Also, your wording paper appears to allow things like

struct [[foo]] S *p; // ok in c n2137, ill-formed in c++
struct T {};
int n = sizeof(struct [[foo]] T); // ok in c n2137, ill-formed in c++

You don't seem to implement those changes; are they bugs in the wording paper? (The relevant C++ rule is [dcl.type.elab]p1: "An attribute-specifier-seq shall not appear in an elaborated-type-specifier unless the latter is the sole constituent of a declaration." Neither of the above cases is prohibited by n2137's 6.7.2.1/6: "An attribute-specifier-seq shall not appear in a struct-or-union-specifier of the form struct-or-union attribute-specifier-seqopt identifier if the struct-or-union-specifier is an incomplete type used in a parameter-declaration or type-name." -- the first case is not in a parameter-declaration or type-name and the second case is not an incomplete type -- and no other rule seems to disallow this.)

include/clang/Basic/LangOptions.def
140 ↗(On Diff #114447)

It's kind of long, but how about DoubleSquareBracketAttributes

include/clang/Driver/Options.td
607 ↗(On Diff #114447)

I wouldn't recommend anyone actually does that, but I'd expect clang to respect their wishes if they ask for it, just like we do for -fms-extensions -fno-declspec.

lib/Parse/ParseDecl.cpp
4219 ↗(On Diff #114447)

OK, but GCC accepts that with a warning in the friend case, and this would also seem to reject valid constructs such as

enum __attribute__((deprecated)) Foo : int;

But... C permits neither the TUK_Declaration nor TUK_Friend cases for enums. The change in your wording proposal appears to be for the TUK_Reference case instead, which you didn't change here.

Also, your wording paper appears to allow things like

struct [[foo]] S *p; // ok in c n2137, ill-formed in c++
struct T {};
int n = sizeof(struct [[foo]] T); // ok in c n2137, ill-formed in c++

You don't seem to implement those changes; are they bugs in the wording paper?

These are bugs in the wording paper which I think I've corrected for the upcoming draft (no N number yet).

(The relevant C++ rule is [dcl.type.elab]p1: "An attribute-specifier-seq shall not appear in an elaborated-type-specifier unless the latter is the sole constituent of a declaration." Neither of the above cases is prohibited by n2137's 6.7.2.1/6: "An attribute-specifier-seq shall not appear in a struct-or-union-specifier of the form struct-or-union attribute-specifier-seqopt identifier if the struct-or-union-specifier is an incomplete type used in a parameter-declaration or type-name." -- the first case is not in a parameter-declaration or type-name and the second case is not an incomplete type -- and no other rule seems to disallow this.)

Yeah, I recognized that after the last meeting and have modified 6.7.2.1p7 to be:

7 An attribute-specifier-seq shall not appear in a struct-or-union-specifier without a struct-declaration-list, except in a declaration of the form:
    struct-or-union attribute-specifier-seq identifier ;
The attributes in the attribute-specifier-seq, if any, are thereafter considered attributes of the struct or union whenever it is named.

I think that resolves both of your concerns, but does it resolve all similar concerns?

include/clang/Basic/LangOptions.def
140 ↗(On Diff #114447)

I can't think of anything better, so I'll go with that.

include/clang/Driver/Options.td
607 ↗(On Diff #114447)

Would you be okay with -fattributes as the flag name, or do you prefer -fdouble-square-bracket-attributes?

lib/Parse/ParseDecl.cpp
4219 ↗(On Diff #114447)

Ah, okay, thank you for the help there, I'll correct (and add another test case, because your example is rejected when passing -fc-attributes).

Updated based on review feedback.

  • Rename CAttributes to DoubleSquareBracketAttributes
  • Added Objective-C test cases to demonstrate no regressed behavior (based on a use-case pointed out during review)
  • Fixed regression with GNU-style attributes in enumerations

I still need to rename the cc1 flag, pending agreement on the name.

aaron.ballman marked 11 inline comments as done.Sep 14 2017, 2:04 PM

Updated based on review feedback.

  • Rename CAttributes to DoubleSquareBracketAttributes
  • Added Objective-C test cases to demonstrate no regressed behavior (based on a use-case pointed out during review)
  • Fixed regression with GNU-style attributes in enumerations

I still need to rename the cc1 flag, pending agreement on the name.

Ping -- the last outstanding issue that I'm aware of is the cc1 flag name, do you have a preference there, Richard? If not, I'll be going with the verbose -fdouble-square-bracket-attributes.

Updated based on review feedback.

  • Rename CAttributes to DoubleSquareBracketAttributes
  • Added Objective-C test cases to demonstrate no regressed behavior (based on a use-case pointed out during review)
  • Fixed regression with GNU-style attributes in enumerations

I still need to rename the cc1 flag, pending agreement on the name.

Ping -- the last outstanding issue that I'm aware of is the cc1 flag name, do you have a preference there, Richard? If not, I'll be going with the verbose -fdouble-square-bracket-attributes.

Post-cppcon-ping.

rsmith requested changes to this revision.Oct 5 2017, 5:09 PM

Mostly looks good, other than the regression in enum parsing.

clang/include/clang/Basic/LangOptions.def
140 ↗(On Diff #115279)

I would probably singularize this to DoubleSquareBracketAttributes. But I'm happy with either.

clang/lib/Parse/ParseDecl.cpp
4223–4225 ↗(On Diff #115279)

This change still looks wrong to me. C++11 attributes are not prohibited in an opaque-enum-declaration.

5973–5974 ↗(On Diff #115279)

Do you anticipate people trying this? Is the concern here that a function declarator can normally be followed by attributes, and so consistency might imply that we allow attributes on the function declarator after an identifier-list instead?

clang/lib/Parse/ParseDeclCXX.cpp
3855–3857 ↗(On Diff #115279)

This uses AS_C2x if -fdouble-square-bracket-attributes is enabled in C++98, which is probably not the desired behavior.

3913 ↗(On Diff #115279)

Accidental lowercasing of an S?

clang/lib/Parse/ParseTentative.cpp
592 ↗(On Diff #115279)

This change is redundant. We wouldn't lex a kw_alignas token outside C++11.

clang/test/CXX/dcl.dcl/dcl.attr/dcl.align/p6.cpp
31 ↗(On Diff #115279)

This looks like the bug I pointed out above. This is a regression; this code is valid.

include/clang/Driver/Options.td
607 ↗(On Diff #114447)

I think -fattributes is problematically non-specific. I think it would be surprising that -fno-attributes does not turn off support for __attribute__, for example.

This revision now requires changes to proceed.Oct 5 2017, 5:09 PM
aaron.ballman edited edge metadata.
aaron.ballman marked 5 inline comments as done.

Corrected review feedback from Richard.

  • Changed the cc1 option to -fdouble-square-bracket-attributes
  • Corrected regression with opaque-enum-declarations
clang/lib/Parse/ParseDecl.cpp
5973–5974 ↗(On Diff #115279)

I don't anticipate this being a common thing for people to try, but it is to prevent an ambiguity:

void fp(a, b) [[foo]] int a, b; { }

Does [[foo]] appertain to the function type or to the declarations a and b. N2165 prohibits attributes from appearing in this location.

clang/lib/Parse/ParseTentative.cpp
592 ↗(On Diff #115279)

Ah, I thought that we would hit this case for _Alignas as well, but I see that's a different keyword. Just to double-check -- we don't expect -fdouble-square-bracket-attributes to enable alignas in C++98, correct?

rsmith accepted this revision.Oct 13 2017, 6:13 PM

Thanks, LGTM :)

../llvm/tools/clang/include/clang/Driver/Options.td
609–616

This is not formatted how we normally format tablegen files.

../llvm/tools/clang/lib/Parse/ParseDecl.cpp
4422

Is this warning appropriate in C? I don't recall whether your proposal permits attributes on enumerators or not.

... in fact, this warning is completely wrong. Fixed in r315784. This should presumably be guarded by an if (getLangOpts().CPlusPlus) with your change, though.

../llvm/tools/clang/lib/Parse/ParseDeclCXX.cpp
3856–3857

I think you can simplify this to LO.CPlusPlus, because LO.DoubleSquareBracketAttributes should always be true if we get here. (Right?)

clang/lib/Parse/ParseTentative.cpp
592 ↗(On Diff #115279)

Right, I would not expect it to affect the set of available keywords.

This revision is now accepted and ready to land.Oct 13 2017, 6:13 PM
aaron.ballman closed this revision.Oct 15 2017, 8:01 AM
aaron.ballman marked an inline comment as done.

I've commit in r315856, thank you for the reviews!

../llvm/tools/clang/include/clang/Driver/Options.td
609–616

I'll fix it up -- I think this was clang-formatted.

../llvm/tools/clang/lib/Parse/ParseDecl.cpp
4422

Is this warning appropriate in C? I don't recall whether your proposal permits attributes on enumerators or not.

Yes, my proposal allows attributes on enumerators. I will fix up with your merge, thanks!

../llvm/tools/clang/lib/Parse/ParseDeclCXX.cpp
3856–3857

Correct, I've simplified.