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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D37436#867287, @rsmith wrote:

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


I do not think that's the correct approach. These are not C++ attributes (for instance, no `using` insanity is allowed, `::` is a new lexing token in C, etc). Also, I don't think it's a good idea to enable all C++11-style attributes in C mode without giving each attribute some appropriate thought (what does `abi_tag` *do* in C mode? What happens with _Noreturn vs [[noreturn]] etc). Also, I'm not comfortable adding a bunch of vendor-specific `gnu::` attributes that GCC does not implement in C yet.

The goal of this is to get a working implementation of the attribute syntax that WG14 has shown strong support for including in C2x (F: 16, N: 3, A: 1) with this syntax (F: 12, N: 3, A: 4). The experimental part where I need feedback is whether the paper needs to change because of implementation experience. So far, that's not been required. The more we drift from what's proposed in N2137, the less useful the implementation experience will be (esp if we basically turn this into a simple "enable C++ attributes everywhere" flag). I can understand not adding a C2x language mode due to the lack of a working draft, but that lack should not be what drives internal implementation details for the feature -- I think the separation of concerns in this patch is valuable.



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


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


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


================
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);
----------------
rsmith wrote:
> This change is unnecessary. `kw_alignas` is not produced by the lexer in modes where we should not parse it.
Ah, good to know. I'll remove.


================
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
----------------
rsmith wrote:
> Likewise.
I'll add a test for this to ensure we don't accidentally allow it in C mode.


https://reviews.llvm.org/D37436