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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
