[clang-format] Add SpaceInParensOption for __attribute__ keyword

[gedare]

Summary

The __attribute__((specifier-list)) currently is formatted based on the SpacesInParensOptions.Other (previously, SpacesInParentheses). This change allows using Other while disabling addition of spaces between the consecutive parens surrounding the specifier-list.

DependsOn: D155239

[owenpan]

clang/include/clang/Format/Format.h

4429–4435

I'm against this as the double parens, IMO, should never be separated.

[owenpan]

clang/include/clang/Format/Format.h

4423–4428

This should be covered by SpacesInParetheses, so we really should not have a special option for __attribute__.

[gedare]

clang/include/clang/Format/Format.h

4423–4428

Currently, the behavior of SpacesInParentheses does this:

__attribute__( ( noreturn ) )

In order to prevent this from happening, it is necessary to add an option to disable it somehow, because I don't see that this kind of spacing should ever be used by anyone, but probably someone does it, and it should be maintained for backward compatibility anyway.

4429–4435

I'm happy to consolidate these to a single option, that only allows to toggle between all spaces and no spaces, i.e.,

__attribute__( ( noreturn ) )  // enabled
__attribute__((noreturn)) // disabled

The style I aim for, uses no spaces around attributes, but likes to have spaces in other places. I will prepare a revision.

clang/lib/Format/TokenAnnotator.cpp

3975

This logic can be simplified, since the parser labels both the left and right parentheses (unlike with the `CastRParen` case.

[gedare]

Merge the two options to just one for attribute parens.

[gedare]

I simplified this to treat the double parens identically, so that it will either inject spaces inside both parens, or not.

Note: This option is necessary to disable SpacesInParens.Other from adding spaces inside of __attribute__((...))

[MyDeveloperDay]

We should never make assumptions about what people do/don't use

If you have this you have to honour it.. if 'SpacesInParentheses ' was true then 'InAttributeSpecifiers' needs to be true by default, shouldn't there be something in the expanding out of SpacesInParentheses

[HazardyKnusperkeks]

clang/include/clang/Format/Format.h

4423–4428

Currently, the behavior of SpacesInParentheses does this:

__attribute__( ( noreturn ) )

In order to prevent this from happening, it is necessary to add an option to disable it somehow, because I don't see that this kind of spacing should ever be used by anyone, but probably someone does it, and it should be maintained for backward compatibility anyway.

And what does clang-format do before your SpacesInParentheses? You should expand the tests to cover the attributes (if they aren't in there already).

[MyDeveloperDay]

We should never make assumptions about what people do/don't use

If you have this you have to honour it.. if 'SpacesInParentheses ' was true then 'InAttributeSpecifiers' needs to be true by default, shouldn't there be something in the expanding out of SpacesInParentheses

[gedare]

I need to fix this to reflect the changes in the parent rev.

clang/include/clang/Format/Format.h

4423–4428

SpacesInParentheses formats like this: __attribute__( ( ... ) )

[gedare]

Rebase onto D155239 and correctly handle deprecated SpacesInParentheses option.

[gedare]

Rebase onto D155239.

[gedare]

remove blank line

[gedare]

Rebase onto D155239.

[gedare]

Add one more attribute test.

[HazardyKnusperkeks]

clang/unittests/Format/ConfigParseTest.cpp

233

You forgot this.

[gedare]

Add missing parser check.

[gedare]

Add a release note.

[gedare]

Rebase to 18

[gedare]

Rebase to main. Fix up use of TT_AttributeParen.

[owenpan]

In D155529#4509686, @MyDeveloperDay wrote:

We should never make assumptions about what people do/don't use

If you have this you have to honour it.. if 'SpacesInParentheses ' was true then 'InAttributeSpecifiers' needs to be true by default, shouldn't there be something in the expanding out of SpacesInParentheses

Because SpacesInParentheses was added 10+ years ago in rGb55acad91c37 and __attribute__((foo)) was not in the unit tests, it's probably a bug that the double parens were not excluded. Not sure whether the users who didn't complain about it really wanted it or simply didn't bother. The only way to find out would be to fix the bug, assuming it indeed was a bug.

When fixing bugs, especially the very old ones, we often have to choose between just fixing the bugs and adding an option to avoid "regressions", and I usually prefer the former. Nevertheless, I would have no problem if this new option is extended to handle all double parens, e.g. if (( i = j )), decltype(( x )), etc.

clang/docs/ClangFormatStyleOptions.rst

**SpacesInParensOptions** (``SpacesInParensCustom``) :versionbadge:`clang-format 17` :ref:`¶ <SpacesInParensOptions>`
   .. code-block:: c++
 
     # Should be declared this way:
     SpacesInParens: Custom
     SpacesInParensOptions:
       InConditionalStatements: true
       Other: true
 
+  * ``bool InAttributeSpecifiers`` Put a space in parentheses of attribute specifiers.
+
+    .. code-block:: c++
+
+       true:                                  false:
+       __attribute__( ( noreturn ) )    vs.     __attribute__((noreturn))
+
   * ``bool InConditionalStatements`` Put a space in parentheses only inside conditional statements
     (``for/if/while/switch...``).
 
     .. code-block:: c++
 
        true:                                  false:
        if ( a )  { ... }              vs.     if (a) { ... }
        while ( i < 5 )  { ... }               while (i < 5) { ... }

clang/docs/ReleaseNotes.rst

 - Add ``dependentSizedExtVectorType``.
 - Add ``macroQualifiedType``.
 
 clang-format
 ------------
 - Add ``AllowBreakBeforeNoexceptSpecifier`` option.
 - Add ``AllowShortCompoundRequirementOnASingleLine`` option.
 - Change ``BreakAfterAttributes`` from ``Never`` to ``Leave`` in LLVM style.
+- Add ``InAttributeSpecifiers`` style option to ``SpacesInParensOptions``
+  to control addition of spaces after the ``__attribute__`` keyword.
 
 libclang
 --------
 
 - Exposed arguments of ``clang::annotate``.
 
 Static Analyzer
 ---------------

clang/include/clang/Format/Format.h

   /// \code
   ///   # Should be declared this way:
   ///   SpacesInParens: Custom
   ///   SpacesInParensOptions:
   ///     InConditionalStatements: true
   ///     Other: true
   /// \endcode
   struct SpacesInParensCustom {
+    /// Put a space in parentheses of attribute specifiers.
+    /// \code
+    ///    true:                                  false:
+    ///    __attribute__( ( noreturn ) )    vs.     __attribute__((noreturn))
+    /// \endcode
+    bool InAttributeSpecifiers;
     /// Put a space in parentheses only inside conditional statements
     /// (``for/if/while/switch...``).
     /// \code
     ///    true:                                  false:
     ///    if ( a )  { ... }              vs.     if (a) { ... }
     ///    while ( i < 5 )  { ... }               while (i < 5) { ... }
     /// \endcode
     bool InConditionalStatements;
     /// Put a space in C style casts.
     /// \code
     ///    true:                                  false:
     ///    x = ( int32 )y                 vs.     x = (int32)y
     /// \endcode
     bool InCStyleCasts;
     /// Put a space in parentheses only if the parentheses are empty i.e. '()'
struct SpacesInParensCustom {
     /// Put a space in parentheses not covered by preceding options.
     /// \code
     ///    true:                                  false:
     ///    t f( Deleted & ) & = delete;   vs.     t f(Deleted &) & = delete;
     /// \endcode
     bool Other;
 
     SpacesInParensCustom()
-        : InConditionalStatements(false), InCStyleCasts(false),
-          InEmptyParentheses(false), Other(false) {}
+        : InAttributeSpecifiers(false), InConditionalStatements(false),
+          InCStyleCasts(false), InEmptyParentheses(false), Other(false) {}
 
-    SpacesInParensCustom(bool InConditionalStatements, bool InCStyleCasts,
+    SpacesInParensCustom(bool InAttributeSpecifiers,
+                         bool InConditionalStatements, bool InCStyleCasts,
                          bool InEmptyParentheses, bool Other)
-        : InConditionalStatements(InConditionalStatements),
+        : InAttributeSpecifiers(InAttributeSpecifiers),
+          InConditionalStatements(InConditionalStatements),
           InCStyleCasts(InCStyleCasts), InEmptyParentheses(InEmptyParentheses),
           Other(Other) {}
 
     bool operator==(const SpacesInParensCustom &R) const {
-      return InConditionalStatements == R.InConditionalStatements &&
+      return InAttributeSpecifiers == R.InAttributeSpecifiers &&
+             InConditionalStatements == R.InConditionalStatements &&
              InCStyleCasts == R.InCStyleCasts &&
              InEmptyParentheses == R.InEmptyParentheses && Other == R.Other;
     }
     bool operator!=(const SpacesInParensCustom &R) const {
       return !(*this == R);
     }
   };
 

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle::SpacesInLineComment &Space) {
 
     if (Space.Maximum != -1u)
       Space.Minimum = std::min(Space.Minimum, Space.Maximum);
   }
 };
 
 template <> struct MappingTraits<FormatStyle::SpacesInParensCustom> {
   static void mapping(IO &IO, FormatStyle::SpacesInParensCustom &Spaces) {
+    IO.mapOptional("InAttributeSpecifiers", Spaces.InAttributeSpecifiers);
     IO.mapOptional("InCStyleCasts", Spaces.InCStyleCasts);
     IO.mapOptional("InConditionalStatements", Spaces.InConditionalStatements);
     IO.mapOptional("InEmptyParentheses", Spaces.InEmptyParentheses);
     IO.mapOptional("Other", Spaces.Other);
   }
 };
 
 template <> struct ScalarEnumerationTraits<FormatStyle::SpacesInParensStyle> {
static void mapping(IO &IO, FormatStyle &Style) {
     }
 
     if (Style.SpacesInParens != FormatStyle::SIPO_Custom &&
         (SpacesInParentheses || SpaceInEmptyParentheses ||
          SpacesInConditionalStatement || SpacesInCStyleCastParentheses)) {
       if (SpacesInParentheses) {
         // set all options except InCStyleCasts and InEmptyParentheses
         // to true for backward compatibility.
+        Style.SpacesInParensOptions.InAttributeSpecifiers = true;
         Style.SpacesInParensOptions.InConditionalStatements = true;
         Style.SpacesInParensOptions.InCStyleCasts =
             SpacesInCStyleCastParentheses;
         Style.SpacesInParensOptions.InEmptyParentheses =
             SpaceInEmptyParentheses;
         Style.SpacesInParensOptions.Other = true;
       } else {
         Style.SpacesInParensOptions = {};

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   }
   // co_await (x), co_yield (x), co_return (x)
   if (Left.isOneOf(tok::kw_co_await, tok::kw_co_yield, tok::kw_co_return) &&
       !Right.isOneOf(tok::semi, tok::r_paren)) {
     return true;
   }
 
   if (Left.is(tok::l_paren) || Right.is(tok::r_paren)) {
-    return (Right.is(TT_CastRParen) ||
-            (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
-               ? Style.SpacesInParensOptions.InCStyleCasts
-               : Style.SpacesInParensOptions.Other;
+    if (Right.is(TT_CastRParen) ||
+        (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen))) {
+      return Style.SpacesInParensOptions.InCStyleCasts;
+    }
+    if (Left.isOneOf(TT_AttributeLParen, TT_AttributeRParen) ||
+        Right.isOneOf(TT_AttributeLParen, TT_AttributeRParen) ||
+        (Left.Previous && Left.Previous->isOneOf(TT_AttributeLParen,
+                                                 TT_AttributeRParen)) ||
+        (Right.Next && Right.Next->isOneOf(TT_AttributeLParen,
+                                           TT_AttributeRParen))) {
+      return Style.SpacesInParensOptions.InAttributeSpecifiers;
+    }
+    return Style.SpacesInParensOptions.Other;
   }
   if (Right.isOneOf(tok::semi, tok::comma))
     return false;
   if (Right.is(tok::less) && Line.Type == LT_ObjCDecl) {
     bool IsLightweightGeneric = Right.MatchingParen &&
                                 Right.MatchingParen->Next &&
                                 Right.MatchingParen->Next->is(tok::colon);
     return !IsLightweightGeneric && Style.ObjCSpaceBeforeProtocolList;

clang/unittests/Format/ConfigParseTest.cpp

TEST(ConfigParseTest, ParsesConfigurationBools) {
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterForeachMacros);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions,
                           AfterFunctionDeclarationName);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions,
                           AfterFunctionDefinitionName);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterIfMacros);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterOverloadedOperator);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, BeforeNonEmptyParentheses);
+  CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InAttributeSpecifiers);
   CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InCStyleCasts);
   CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InConditionalStatements);
   CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InEmptyParentheses);
   CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, Other);
 }
 
 #undef CHECK_PARSE_BOOL
 
#undef CHECK_ALIGN_CONSECUTIVE
   // For backward compatibility:
   Style.SpacesInParens = FormatStyle::SIPO_Never;
   Style.SpacesInParensOptions = {};
   CHECK_PARSE("SpacesInParentheses: true", SpacesInParens,
               FormatStyle::SIPO_Custom);
   Style.SpacesInParens = FormatStyle::SIPO_Never;
   Style.SpacesInParensOptions = {};
   CHECK_PARSE("SpacesInParentheses: true", SpacesInParensOptions,
-              FormatStyle::SpacesInParensCustom(true, false, false, true));
+              FormatStyle::SpacesInParensCustom(true, true, false, false,
+                  true));
   Style.SpacesInParens = FormatStyle::SIPO_Never;
   Style.SpacesInParensOptions = {};
   CHECK_PARSE("SpacesInConditionalStatement: true", SpacesInParensOptions,
-              FormatStyle::SpacesInParensCustom(true, false, false, false));
+              FormatStyle::SpacesInParensCustom(false, true, false, false,
+                  false));
   Style.SpacesInParens = FormatStyle::SIPO_Never;
   Style.SpacesInParensOptions = {};
   CHECK_PARSE("SpacesInCStyleCastParentheses: true", SpacesInParensOptions,
-              FormatStyle::SpacesInParensCustom(false, true, false, false));
+              FormatStyle::SpacesInParensCustom(false, false, true, false,
+                  false));
   Style.SpacesInParens = FormatStyle::SIPO_Never;
   Style.SpacesInParensOptions = {};
   CHECK_PARSE("SpaceInEmptyParentheses: true", SpacesInParensOptions,
-              FormatStyle::SpacesInParensCustom(false, false, true, false));
+              FormatStyle::SpacesInParensCustom(false, false, false, true,
+                  false));
   Style.SpacesInParens = FormatStyle::SIPO_Never;
   Style.SpacesInParensOptions = {};
 
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
   CHECK_PARSE("BasedOnStyle: LLVM\nColumnLimit: 1234", ColumnLimit, 1234u);
 

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConfigurableSpacesInParens) {
   verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
   verifyFormat("void __attribute__((naked)) foo(int bar)", Spaces);
   verifyFormat("void f() __attribute__((asdf));", Spaces);
 
   Spaces.SpacesInParens = FormatStyle::SIPO_Custom;
   Spaces.SpacesInParensOptions = {};
   Spaces.SpacesInParensOptions.Other = true;
   Spaces.SpacesInParensOptions.InConditionalStatements = true;
+  Spaces.SpacesInParensOptions.InAttributeSpecifiers = true;
   verifyFormat("do_something( ::globalVar );", Spaces);
   verifyFormat("call( x, y, z );", Spaces);
   verifyFormat("call();", Spaces);
   verifyFormat("std::function<void( int, int )> callback;", Spaces);
   verifyFormat("void inFunction() { std::function<void( int, int )> fct; }",
                Spaces);
   verifyFormat("while ( (bool)1 )\n"
                "  continue;",
verifyFormat("switch (x) {\n"
                "default:\n"
                "  break;\n"
                "}",
                Spaces);
   verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
   verifyFormat("void __attribute__((naked)) foo(int bar)", Spaces);
   verifyFormat("void f( ) __attribute__((asdf));", Spaces);
 
+  Spaces.SpacesInParensOptions.InAttributeSpecifiers = true;
+  verifyFormat("SomeType *__attribute__( ( attr ) ) *a = NULL;", Spaces);
+  verifyFormat("void __attribute__( ( naked ) ) foo(int bar)", Spaces);
+  verifyFormat("void f( ) __attribute__( ( asdf ) );", Spaces);
+  Spaces.SpacesInParensOptions.InAttributeSpecifiers = false;
+
   // Run the first set of tests again with:
   Spaces.SpaceAfterCStyleCast = true;
   verifyFormat("call(x, y, z);", Spaces);
   verifyFormat("call( );", Spaces);
   verifyFormat("std::function<void(int, int)> callback;", Spaces);
   verifyFormat("while (( bool ) 1)\n"
                "  continue;",
                Spaces);