[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D37436#870305, @rsmith wrote:

> 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-seq//opt //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?



================
Comment at: include/clang/Basic/LangOptions.def:140
 
+LANGOPT(CAttributes       , 1, 0, "'[[]]' attributes extension to C")
+
----------------
rsmith wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > Can we give this a more general name, then enable it under `CPlusPlus` too? That's what we do for other similar features.
> > I'm not keen on naming it 'Attributes', but the other names are worse (`GeneralAttributes`, `StandardAttributes`). Do you have a suggestion?
> It's kind of long, but how about `DoubleSquareBracketAttributes`
I can't think of anything better, so I'll go with that.


================
Comment at: include/clang/Driver/Options.td:607
 
+def fc_attributes : Flag<["-"], "fc-attributes">, Group<f_Group>,
+  Flags<[DriverOption, CC1Option]>, HelpText<"Enable '[[]]' attributes in C">;
----------------
rsmith wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > 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.
> > 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?
> 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`.
Would you be okay with `-fattributes` as the flag name, or do you prefer `-fdouble-square-bracket-attributes`?


================
Comment at: lib/Parse/ParseDecl.cpp:4219
+
+    // Attributes are prohibited in this location in C2x (and forward
+    // declarations are prohibited in C++).
----------------
rsmith wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > 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;
> > > ```
> > > ?
> > 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.
> 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 `enum`s. The change in your wording proposal appears to be for the `TUK_Reference` case instead, which you didn't change here.
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).


https://reviews.llvm.org/D37436