[clang-format] Add a SpaceAroundPointerQualifiers style option

[arichardson]

Summary

Some projects (e.g. FreeBSD) align pointers to the right but expect a
space between the '*' and any pointer qualifiers such as const. To handle
these cases this patch adds a new config option SpaceAroundPointerQualifiers
that can be used to configure whether spaces need to be added before/after
pointer qualifiers.

PointerAlignment = Right
SpaceAroundPointerQualifiers = Default/After:
void *const *x = NULL;
SpaceAroundPointerQualifiers = Before/Both
void * const *x = NULL;

PointerAlignment = Left
SpaceAroundPointerQualifiers = Default/Before:
void* const* x = NULL;
SpaceAroundPointerQualifiers = After/Both
void* const * x = NULL;

PointerAlignment = Middle
SpaceAroundPointerQualifiers = Default/Before/After/Both:
void * const * x = NULL;

[jrtc27]

clang/lib/Format/TokenAnnotator.cpp

2874–2889

[arichardson]

clang/lib/Format/TokenAnnotator.cpp

2874–2889

I feel like moving it here could in theory miss some cases. Also the condition is already barely comprehensible (I didn't attempt to understand which special cases all these conditions are for) and I don't feel like making it more complex.

If clang-format has identified that this */& token is a pointer/reference, and the next token is something that can be paresed as a pointer qualifier, shouldn't we trust the parser and simply look at the format option? It also IMO makes the code slightly easier to understand.

[arichardson]

clang/lib/Format/TokenAnnotator.cpp

2874–2889

Never mind, I thought your suggestion it was inside nested parens. It's equivalent just avoids the braces.

[jrtc27]

clang/lib/Format/TokenAnnotator.cpp

2874–2889

It's a series of A || B || C || ..., and you just added an if (X) return true; above, so changing it to A || B || C || X || ... is entirely equivalent, no need to understand what any of the other expressions in the giant OR are.

2874–2889

Uh of course with || added on the end of that new sub-expression, managed to omit that...

[arichardson]

merge if with existing condition

[arichardson]

• Add a multi-variable-decl test (depends on D88239)

[MyDeveloperDay]

clang/include/clang/Format/Format.h

2181

I wonder if a request would ever say they wanted this Space before to be Left/Middle/Right? perhaps this should not be a bool?

void* const *x;
void * const *x;
void *const *x;

void* const *x;
void * const *x;
void *const *x;

void* const * x;
void * const * x;
void *const * x;

void* const* x;
void * const* x;
void *const* x;

[arichardson]

clang/include/clang/Format/Format.h

2181

Shouldn't be too hard to implement so why not. How about PointerAlignmentForQualifiers as the setting name?

We will have to ensure that the default value matches PointerAlignment if it's not set since otherwise you'd get really weird code formatting after updating clang-format if you don't change the config file.
Are there and other settings that behave this way? If not I think I'd rather keep the boolean option.

[mzeren-vmw]

Perhaps PointerAlignment should be an enum of Left, Middle, Right?
We maintain an internal patch that adds "ReferenceAlignment" as separate from PointerAlignment. So one could have:

PointerAlignment: Middle
ReferenceAlignment: Left

[arichardson]

Use an enum config option instead

[MyDeveloperDay]

LGTM

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
        void f () {
          if (true) {
            f ();
          }
        }
 
 
 
+**SpaceBeforePointerQualifiers** (``bool``)
+  If ``true``, spaces will be added before pointer qualifiers. This only
+  changes the format when pointers are aligned to the right.
+
+  .. code-block:: c++
+
+     true:                                  false:
+     void * const x = NULL;         vs.     void *const x = NULL;
+
 **SpaceBeforeRangeBasedForLoopColon** (``bool``)
   If ``false``, spaces will be removed before range-based for loop
   colon.
 
   .. code-block:: c++
 
      true:                                  false:
      for (auto v : values) {}       vs.     for(auto v: values) {}

clang/include/clang/Format/Format.h

enum SpaceBeforeParensOptions {
     ///    }
     /// \endcode
     SBPO_Always
   };
 
   /// Defines in which cases to put a space before opening parentheses.
   SpaceBeforeParensOptions SpaceBeforeParens;
 
+  /// If ``true``, spaces will be added before pointer qualifiers. This only
+  /// changes the format when pointers are aligned to the right.
+  /// \code
+  ///    true:                                  false:
+  ///    void * const x = NULL;         vs.     void *const x = NULL;
+  /// \endcode
+  bool SpaceBeforePointerQualifiers;
+
   /// If ``false``, spaces will be removed before range-based for loop
   /// colon.
   /// \code
   ///    true:                                  false:
   ///    for (auto v : values) {}       vs.     for(auto v: values) {}
   /// \endcode
   bool SpaceBeforeRangeBasedForLoopColon;
 
return AccessModifierOffset == R.AccessModifierOffset &&
            SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
            SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
            SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
            SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
            SpaceBeforeCtorInitializerColon ==
                R.SpaceBeforeCtorInitializerColon &&
            SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
+           SpaceBeforePointerQualifiers == R.SpaceBeforePointerQualifiers &&
            SpaceBeforeRangeBasedForLoopColon ==
                R.SpaceBeforeRangeBasedForLoopColon &&
            SpaceInEmptyBlock == R.SpaceInEmptyBlock &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
            SpacesInAngles == R.SpacesInAngles &&
            SpacesInConditionalStatement == R.SpacesInConditionalStatement &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&

clang/lib/Format/Format.cpp

IO.mapOptional("SpaceBeforeAssignmentOperators",
                    Style.SpaceBeforeAssignmentOperators);
     IO.mapOptional("SpaceBeforeCpp11BracedList",
                    Style.SpaceBeforeCpp11BracedList);
     IO.mapOptional("SpaceBeforeCtorInitializerColon",
                    Style.SpaceBeforeCtorInitializerColon);
     IO.mapOptional("SpaceBeforeInheritanceColon",
                    Style.SpaceBeforeInheritanceColon);
     IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
+    IO.mapOptional("SpaceBeforePointerQualifiers",
+                   Style.SpaceBeforePointerQualifiers);
     IO.mapOptional("SpaceBeforeRangeBasedForLoopColon",
                    Style.SpaceBeforeRangeBasedForLoopColon);
     IO.mapOptional("SpaceInEmptyBlock", Style.SpaceInEmptyBlock);
     IO.mapOptional("SpaceInEmptyParentheses", Style.SpaceInEmptyParentheses);
     IO.mapOptional("SpacesBeforeTrailingComments",
                    Style.SpacesBeforeTrailingComments);
     IO.mapOptional("SpacesInAngles", Style.SpacesInAngles);
     IO.mapOptional("SpacesInConditionalStatement",
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
+  LLVMStyle.SpaceBeforePointerQualifiers = false;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
   LLVMStyle.BitFieldColonSpacing = FormatStyle::BFCS_Both;
   LLVMStyle.SpacesInAngles = false;
   LLVMStyle.SpacesInConditionalStatement = false;
 

clang/lib/Format/TokenAnnotator.cpp

return (Left.Tok.isLiteral() ||
                (Left.NestingLevel == 0 ||
                 (Left.NestingLevel == 1 && Line.First->is(tok::kw_for)))))));
   }
   if (Right.is(TT_FunctionTypeLParen) && Left.isNot(tok::l_paren) &&
       (!Left.is(TT_PointerOrReference) ||
        (Style.PointerAlignment != FormatStyle::PAS_Right &&
         !Line.IsMultiVariableDeclStmt)))
     return true;
-  if (Left.is(TT_PointerOrReference))
+  if (Left.is(TT_PointerOrReference)) {
+    if (Style.SpaceBeforePointerQualifiers &&
+        Right.canBePointerOrReferenceQualifier())
+      return true;
     return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
            (Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
             !Right.is(TT_StartOfName)) ||
            (Right.is(tok::l_brace) && Right.is(BK_Block)) ||
            (!Right.isOneOf(TT_PointerOrReference, TT_ArraySubscriptLSquare,
                            tok::l_paren) &&
             (Style.PointerAlignment != FormatStyle::PAS_Right &&
              !Line.IsMultiVariableDeclStmt) &&
             Left.Previous &&
             !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon,
                                     tok::l_square));
+  }
   // Ensure right pointer alignement with ellipsis e.g. int *...P
   if (Left.is(tok::ellipsis) && Left.Previous &&
       Left.Previous->isOneOf(tok::star, tok::amp, tok::ampamp))
     return Style.PointerAlignment != FormatStyle::PAS_Right;
 
   if (Right.is(tok::star) && Left.is(tok::l_paren))
     return false;
   if (Left.is(tok::star) && Right.isOneOf(tok::star, tok::amp, tok::ampamp))

clang/unittests/Format/FormatTest.cpp

verifyFormat("{\n"
                NoSpaceStyle);
   verifyFormat("switch (x) {\n"
                "case 1:\n"
                "default:\n"
                "}",
                NoSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforePointerQualifiers) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.AttributeMacros.push_back("qualified");
+  verifyFormat("SomeType *const *a = NULL;", Spaces);
+  verifyFormat("SomeType *volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType *qualified *a = NULL;", Spaces);
+  verifyFormat("std::vector<SomeType *const> x;", Spaces);
+  verifyFormat("std::vector<SomeType *qualified> x;", Spaces);
+  verifyFormat("std::vector<SomeVar * NotAQualifier> x;", Spaces);
+
+  Spaces.SpaceBeforePointerQualifiers = true;
+  verifyFormat("SomeType * const *a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified *a = NULL;", Spaces);
+  verifyFormat("std::vector<SomeType * const> x;", Spaces);
+  verifyFormat("std::vector<SomeType * qualified> x;", Spaces);
+  verifyFormat("std::vector<SomeVar * NotAQualifier> x;", Spaces);
+
+  // Check that setting SpaceBeforePointerQualifiers doesn't result in extra
+  // spaces for PAS_Left/PAS_Middle.
+  Spaces.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("SomeType* const* a = NULL;", Spaces);
+  verifyFormat("SomeType* volatile* a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr))* a = NULL;", Spaces);
+  verifyFormat("SomeType* qualified* a = NULL;", Spaces);
+  verifyFormat("std::vector<SomeType* const> x;", Spaces);
+  verifyFormat("std::vector<SomeType* qualified> x;", Spaces);
+  verifyFormat("std::vector<SomeVar * NotAQualifier> x;", Spaces);
+
+  Spaces.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("SomeType * const * a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified * a = NULL;", Spaces);
+  verifyFormat("std::vector<SomeType * const> x;", Spaces);
+  verifyFormat("std::vector<SomeType * qualified> x;", Spaces);
+  verifyFormat("std::vector<SomeVar * NotAQualifier> x;", Spaces);
+}
+
 TEST_F(FormatTest, AlignConsecutiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.AlignConsecutiveAssignments = true;
   Style.AlignConsecutiveDeclarations = true;
   Style.AlignConsecutiveMacros = false;
 
   verifyFormat("#define a 3\n"
                "#define bbbb 4\n"
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceAfterLogicalNot);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
+  CHECK_PARSE_BOOL(SpaceBeforePointerQualifiers);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
   CHECK_PARSE_BOOL(SpaceBeforeSquareBrackets);
   CHECK_PARSE_BOOL(UseCRLF);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterFunction);