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

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

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

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



================
Comment at: clang/include/clang/Basic/LangOptions.def:140
 
+LANGOPT(DoubleSquareBracketsAttributes, 1, 0, "'[[]]' attributes extension for all language standard modes")
+
----------------
I would probably singularize this to `DoubleSquareBracketAttributes`. But I'm happy with either.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:4223-4225
+    // C++11 attributes are prohibited in this location.
+    if (ParsedCXX11Attrs)
+      ProhibitAttributes(attrs);
----------------
This change still looks wrong to me. C++11 attributes are not prohibited in an //opaque-enum-declaration//.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5973-5974
+
+    // If there are attributes following the identifier list, parse them and 
+    // prohibit them.
+    MaybeParseCXX11Attributes(FnAttrs);
----------------
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?


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3855-3857
+  AttributeList::Syntax Syntax = getLangOpts().CPlusPlus11
+                                     ? AttributeList::AS_CXX11
+                                     : AttributeList::AS_C2x;
----------------
This uses `AS_C2x` if `-fdouble-square-bracket-attributes` is enabled in C++98, which is probably not the desired behavior.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3913
 
-/// ParseCXX11AttributeSpecifier - Parse a C++11 attribute-specifier.
+/// ParseCXX11Attributespecifier - Parse a C++11 or C2x attribute-specifier.
 ///
----------------
Accidental lowercasing of an `S`?


================
Comment at: clang/lib/Parse/ParseTentative.cpp:592
                                   bool OuterMightBeMessageSend) {
-  if (Tok.is(tok::kw_alignas))
+  if (Tok.is(tok::kw_alignas) && getLangOpts().CPlusPlus11)
     return CAK_AttributeSpecifier;
----------------
This change is redundant. We wouldn't lex a `kw_alignas` token outside C++11.


================
Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.align/p6.cpp:31
 
-enum alignas(2) E : char; // expected-note {{declared with 'alignas' attribute here}}
-enum E : char {}; // expected-error {{'alignas' must be specified on definition if it is specified on any declaration}}
+enum alignas(2) E : char; // expected-error {{an attribute list cannot appear here}}
+enum E : char {};
----------------
This looks like the bug I pointed out above. This is a regression; this code is valid.


================
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:
> > 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`?
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.


https://reviews.llvm.org/D37436