[clang-format] Fix spaces around */& in multi-variable declarations

[arichardson]

Summary

While writing tests for D88227, I noticed that multi-variable declarations
such as SomeType *qualified *a = NULL, *const *b; were being formatted
weirdly for Left and Middle pointer alignment. This change attempts to make
this more consistent and ensures there is always a space between the */&
and the variable name.

Before (with PAS_Left):
SomeType * const *a = NULL, * const *b;
After:
SomeType* const* a = NULL, * const* b;

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2889

I wish more of these horrendous expressions all over the code were written like this, early exit and with a comment! have an A+

clang/unittests/Format/FormatTest.cpp

6636

huh! the original tests doesn't look like LEFT aligned but now we also lose the alignment, should we care I wonder?

6639

I don't get why *q would be * q

[arichardson]

clang/unittests/Format/FormatTest.cpp

6636

I looked at the git log and it seems like it was changed from left to right in https://github.com/llvm/llvm-project/commit/bea1ab46d9ffdfc50108580c712596a54323a94c

6636

Losing the alignment is annoying but I don't quite understand why it changed. Hopefully there aren't too many multi line multi variable declarations.

6639

Yeah I'm not sure what the best way to format multi-variable declarations with left alignment. We usually have a space before the name so I thought I'd do the same here (even though it looks a bit odd).

[arichardson]

ping?

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

6636

Again how is this even left alignment anyway!

6636

I'm looking back an @djasper change, and it seems we are basically reverting the behavior it so I feel a little uncomfortable, I'm not sure how to proceed,

having said that I think that change was also wrong because its supposed to be left aligned and basically its not!

At this point I'd personally prefer to wait for a higher power, maybe if @sammccall or @klimek are listening they could comment.

I'm going to pull the review into my local branch and see what impact it has on the code I look after. I also often run my code through those areas of clang that are fully clang-formatted to see what the impact will be.

6639

Actually this case is odd, because its supposed to be left aligned and we are getting right alignment anyway... ignore my comment on the right

[MyDeveloperDay]

I think this patch needs rebasing

[djasper]

Sorry for being a bit late here and thanks @klimek for bringing this to my attention.
This has been years ago, but if I reconstruct my thinking (and look at the test cases), then I'd say that "left" alignment should not be applied to multi-variable decl statements ever.

int* a, b;

Just looks too much like it's declaring two int pointers when it is not. And it leads to all sorts of weirdness when doing line wrapping.
So, I'd really like to keep the current behavior.
If you'd want to change it because it makes a difference for someone's codebase, I'd almost lean towards introducing an additional PointerAlignmentStyle PAS_LeftEvenForMutliVarDeclStatements :).

clang/lib/Format/TokenAnnotator.cpp

case tok::colon:
         // This handles a special macro in ObjC code where selectors including
         // the colon are passed as macro arguments.
         Tok->setType(TT_ObjCMethodExpr);
       } else if (Contexts.back().ContextKind == tok::l_paren) {
         Tok->setType(TT_InlineASMColon);
       }
       break;
     case tok::pipe:
-    case tok::amp:
       // | and & in declarations/type expressions represent union and
       // intersection types, respectively.
       if (Style.Language == FormatStyle::LK_JavaScript &&
           !Contexts.back().IsExpression)
         Tok->setType(TT_JsTypeOperator);
       break;
+    case tok::amp:
+      if (Style.Language == FormatStyle::LK_JavaScript &&
+          !Contexts.back().IsExpression) {
+        Tok->setType(TT_JsTypeOperator);
+        break;
+      }
+      LLVM_FALLTHROUGH; // Handle multi-variable declarations of reference type
+    case tok::star:
+      if (Style.isCpp() && Contexts.back().FirstStartOfName &&
+          Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt) {
+        // '*' or '&' after a comma is always a pointer or reference if
+        // we are in a context declaring multiple variables.
+        FormatToken *Prev = Tok->getPreviousNonComment();
+        if (Prev && Prev->is(tok::comma)) {
+          Tok->setType(TT_PointerOrReference);
+          Line.IsMultiVariableDeclStmt = true;
+        }
+      }
+      break;
     case tok::kw_if:
     case tok::kw_while:
       if (Tok->is(tok::kw_if) && CurrentToken &&
           CurrentToken->isOneOf(tok::kw_constexpr, tok::identifier))
         next();
       if (CurrentToken && CurrentToken->is(tok::l_paren)) {
         next();
         if (!parseParens(/*LookForDecls=*/true))
if (Right.is(TT_PointerOrReference)) {
     if (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) {
       if (!Left.MatchingParen)
         return true;
       FormatToken *TokenBeforeMatchingParen =
           Left.MatchingParen->getPreviousNonComment();
       if (!TokenBeforeMatchingParen || !Left.is(TT_TypeDeclarationParen))
         return true;
     }
-    return (Left.Tok.isLiteral() ||
-            (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
-             (Style.PointerAlignment != FormatStyle::PAS_Left ||
-              (Line.IsMultiVariableDeclStmt &&
-               (Left.NestingLevel == 0 ||
-                (Left.NestingLevel == 1 && Line.First->is(tok::kw_for)))))));
+    if (Left.Tok.isLiteral())
+      return true;
+    // We group multiple '*'/'&' tokens for all pointer alignment settings.
+    // TODO: why is the l_paren needed?
+    if (Left.isOneOf(TT_PointerOrReference, tok::l_paren))
+      return false;
+    // We always place a space after the comma in multi-variable declarations.
+    if (Line.IsMultiVariableDeclStmt && Left.is(tok::comma))
+      return true;
+    return Style.PointerAlignment != FormatStyle::PAS_Left;
   }
   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))
     return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
            (Style.SpaceBeforePointerQualifiers &&
             Right.canBePointerOrReferenceQualifier()) ||
            (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 &&
+            Style.PointerAlignment != FormatStyle::PAS_Right && 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))

clang/unittests/Format/FormatTest.cpp

verifyFormat("aaaaaaaaa *a = aaaaaaaaaaaaaaaaaaa, *b = bbbbbbbbbbbbbbbbbbb,\n"
                "          *c = ccccccccccccccccccc, *d = ddddddddddddddddddd;");
   verifyFormat("aaaaaaaaa ***a = aaaaaaaaaaaaaaaaaaa, ***b = bbbbbbbbbbbbbbb,\n"
                "          ***c = ccccccccccccccccccc, ***d = ddddddddddddddd;");
 
   FormatStyle Style = getGoogleStyle();
   Style.PointerAlignment = FormatStyle::PAS_Left;
   Style.DerivePointerAlignment = false;
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-               "    *aaaaaaaaaaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaa,\n"
-               "    *b = bbbbbbbbbbbbbbbbbbb;",
+               "    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaa,\n"
+               "      * b = bbbbbbbbbbbbbbbbbbb;",
                Style);
-  verifyFormat("aaaaaaaaa *a = aaaaaaaaaaaaaaaaaaa, *b = bbbbbbbbbbbbbbbbbbb,\n"
-               "          *b = bbbbbbbbbbbbbbbbbbb, *d = ddddddddddddddddddd;",
+  verifyFormat("aaaaaaaaa* a = aaaaaaaaaaaaaaaaaaa, * b = bbbbbbbbbbbbbbbbbb,\n"
+               "           * b = bbbbbbbbbbbbbbbbbbb, * d = dddddddddddddddd;",
                Style);
   verifyFormat("vector<int*> a, b;", Style);
-  verifyFormat("for (int *p, *q; p != q; p = p->next) {\n}", Style);
+  verifyFormat("for (int* p, * q; p != q; p = p->next) {\n}", Style);
+  verifyFormat("char* x, * const* y = NULL;", Style);
+  verifyFormat("char** x, ** y = NULL;", Style);
+  verifyFormat("int x, * xp = &x, & xr = x, *& xpr = xp, * const& xcpr = xp;",
+               Style);
+  Style.PointerAlignment = FormatStyle::PAS_Right;
+  verifyFormat("for (int *p, *q; p != q; p = p->next) {\n}", Style);
+  verifyFormat("char *x, *const *y = NULL;", Style);
+  verifyFormat("char **x, **y = NULL;", Style);
+  verifyFormat("int x, *xp = &x, &xr = x, *&xpr = xp, *const &xcpr = xp;",
+               Style);
+  Style.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("for (int * p, * q; p != q; p = p->next) {\n}", Style);
+  verifyFormat("char * x, * const * y = NULL;", Style);
+  verifyFormat("char ** x, ** y = NULL;", Style);
+  verifyFormat("int x, * xp = &x, & xr = x, *& xpr = xp, * const & xcpr = xp;",
+               Style);
 }
 
 TEST_F(FormatTest, ConditionalExpressionsInBrackets) {
   verifyFormat("arr[foo ? bar : baz];");
   verifyFormat("f()[foo ? bar : baz];");
   verifyFormat("(a + b)[foo ? bar : baz];");
   verifyFormat("arr[foo ? (4 > 5 ? 4 : 5) : 5 < 5 ? 5 : 7];");
 }