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

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

rsmith added a comment.

If this is just supposed to be an experiment to get feedback on the feature, then I don't think we should be treating it as a different attribute syntax at all. Rather, I think we just want to permit C++11 attributes to be parsed in other language modes. If/when this becomes part of some future C working draft, I think that's the time to have a separate attribute syntax with a distinct set of valid unqualified attribute names.



================
Comment at: include/clang/Basic/LangOptions.def:140
 
+LANGOPT(CAttributes       , 1, 0, "'[[]]' attributes extension to C")
+
----------------
Can we give this a more general name, then enable it under `CPlusPlus` too? That's what we do for other similar features.


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


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


================
Comment at: lib/Parse/ParseDeclCXX.cpp:3939-3940
                                           SourceLocation *endLoc) {
-  if (Tok.is(tok::kw_alignas)) {
+  bool CXXAttr = getLangOpts().CPlusPlus11;
+  if (CXXAttr && Tok.is(tok::kw_alignas)) {
     Diag(Tok.getLocation(), diag::warn_cxx98_compat_alignas);
----------------
This change is unnecessary. `kw_alignas` is not produced by the lexer in modes where we should not parse it.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:3947
+  assert(Tok.is(tok::l_square) && NextToken().is(tok::l_square) &&
+         "Not a standards-based attribute list");
 
----------------
`[[`... in C is not a standards-based attribute list. I'd revert this change too.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:3956
   IdentifierInfo *CommonScopeName = nullptr;
-  if (Tok.is(tok::kw_using)) {
+  if (CXXAttr && Tok.is(tok::kw_using)) {
     Diag(Tok.getLocation(), getLangOpts().CPlusPlus1z
----------------
Likewise.


https://reviews.llvm.org/D37436