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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D37436#869851, @hfinkel wrote:

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



================
Comment at: include/clang/Basic/LangOptions.def:140
 
+LANGOPT(CAttributes       , 1, 0, "'[[]]' attributes extension to C")
+
----------------
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`


================
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">;
----------------
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`.


================
Comment at: lib/Parse/ParseDecl.cpp:4219
+
+    // Attributes are prohibited in this location in C2x (and forward
+    // declarations are prohibited in C++).
----------------
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.


https://reviews.llvm.org/D37436