[PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

Fri Jun 9 11:33:05 PDT 2023

aaron.ballman added subscribers: beanz, clang-vendors.
aaron.ballman added a comment.

In D151683#4384017 <https://reviews.llvm.org/D151683#4384017>, @erichkeane wrote:

> In D151683#4382408 <https://reviews.llvm.org/D151683#4382408>, @philnik wrote:
>
>> No. I guess, since you are asking I should write one for this? Only for the removal of `-fdouble-square-bracket-attributes`, or also for back porting this?
>
> The RFC I would expect for "allow C/C++ style attributes as an extension in previous modes".  This is a pretty significant feature to allow as an extension, particularly since our two main alternative compilers (G++/MSVC) don't have this as an extension. I'd be curious how the other C compilers do this as well.

I think this does warrant an RFC because it impacts both C++ and C, but I'm hoping the RFC is uncontroversial. It's mostly to establish what the use cases are for needing the extension. The feature is conforming as an extension to both languages, and I don't know of any breakage that would come from enabling it by default. I'm not certain whether we want to remove the feature flag immediately or whether we'd like to give one release of warning about it being removed (I'll defer to whatever @MaskRay thinks is reasonable) -- that search is compelling for why it's safe to remove the option, but there's plenty of proprietary code which we don't know about, so it's possible the option is still used. I'd be especially curious to know if anyone is using `-fno-double-square-bracket-attributes` to disable the feature in a mode where it would otherwise be enabled. I'm adding the `clang-vendors` group as a subscriber as an early warning that removing the command line option could be a potentially breaking change.

In terms of implementation work, there's still some minor stuff to address. Also, please be sure to also add a release note about the changes, and a potentially breaking entry for removing the command line option (assuming we elect to go that route).

================
Comment at: clang/docs/LanguageExtensions.rst:1434
 Designated initializers (N494)                                          C99           C89
 Array & element qualification (N2607)                                   C2x           C89
+====================================== ================================ ============= =============
----------------
You need to add an entry here as well, as this also extends C.

================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:553
 def err_while_loop_outside_of_a_function : Error<
-  "while loop outside of a function">; 
+  "while loop outside of a function">;
 def err_brackets_go_after_unqualified_id : Error<
----------------
Spurious whitespace change.

================
Comment at: clang/include/clang/Driver/Options.td:3530
   Values<"none,cf,cf-nochecks">;
-def mcpu_EQ : Joined<["-"], "mcpu=">, Group<m_Group>, 
+def mcpu_EQ : Joined<["-"], "mcpu=">, Group<m_Group>,
   HelpText<"For a list of available CPUs for the target use '-mcpu=help'">;
----------------
Spurious whitespace change.

================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1196
       // Add include/gpu-none-libc/* to our system include path. This lets us use
-      // GPU-specific system headers first. 
+      // GPU-specific system headers first.
       // TODO: We need to find a way to make these headers compatible with the
----------------
Spurious whitespace change.

================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4477
   SourceLocation OpenLoc = Tok.getLocation();
-  Diag(OpenLoc, diag::warn_cxx98_compat_attribute);
+  Diag(OpenLoc, getLangOpts().CPlusPlus11 ? diag::warn_cxx98_compat_attribute
+                : getLangOpts().CPlusPlus ? diag::warn_ext_cxx11_attributes
----------------
Missing the same "not compatible with standards before C2x" warning as for C++ (might want to reword the C++ warning at the same time to fit the newer style)

================
Comment at: clang/test/OpenMP/assumes_messages_attr.c:57
 [[omp::directive(begin assumes ext_abc)]];
-
----------------
Spurious removal.

================
Comment at: clang/test/Parser/asm.c:14
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
-  asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
+  asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}}

----------------
Spurious whitespace change

================
Comment at: clang/test/Parser/cxx-decl.cpp:316
 #if __cplusplus >= 201103L
-// expected-error at +3 {{expected}}
+// expected-error at +2 {{expected}}
 // expected-error at -3 {{expected ';' after top level declarator}}
----------------
Huh... I wasn't expecting to see a change here because there's no attribute nearby. Probably fine, but still a bit curious.

================
Comment at: clang/test/Parser/cxx-decl.cpp:322
 #endif
-
----------------
Spurious change.

================
Comment at: clang/test/Parser/objc-attr.m:17

-[[clang::objc_protocol_requires_explicit_implementation]] 
+[[clang::objc_protocol_requires_explicit_implementation]]
 @protocol Baz
----------------
Spurious change.

================
Comment at: clang/test/ParserHLSL/group_shared.hlsl:14
-// expected-error at +1 {{expected expression}}
 float groupshared [[]] i = 12;

----------------
philnik wrote:
> Should this also get an extension warning/should attributes be disabled for HLSL?
CC @beanz 

I was wondering the same thing. :-)

================
Comment at: clang/test/Sema/attr-type-safety.c:45
 [[clang::pointer_with_type_tag(c,1,2)]] void C_func(void *ptr, int tag);
-
----------------
Spurious change.

================
Comment at: clang/test/Sema/overloadable.c:77

-// Validate that the invalid function doesn't stay overloadable. 
+// Validate that the invalid function doesn't stay overloadable.
 int __attribute__((overloadable)) invalid(); // expected-error{{'overloadable' function 'invalid' must have a prototype}}
----------------
Spurious change.

================
Comment at: clang/test/SemaCXX/attr-cxx-disabled.cpp:1
-// RUN: %clang_cc1 -fsyntax-only -fno-double-square-bracket-attributes -verify -pedantic -std=c++11 -DERRORS %s
-// RUN: %clang_cc1 -fsyntax-only -fdouble-square-bracket-attributes -verify -pedantic -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -std=c++11 -DERRORS %s

----------------
This whole test file can go away -- we no longer let you disable attributes with this extension.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151683/new/

https://reviews.llvm.org/D151683