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