[clang-format] PR48916 PointerAlignment not working when using C++20 init-statement in for loop

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=48916

Left and Right Alignment inside a loop is misaligned.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

3052

Is this valid code? Or did we just wrongly assign PointerOrReference? I'd say after that there can not be a literal in valid code, thus we do not need to handle it.

3065

Do we need this just for auto? What when auto is replaced with int?

3070

Drop the outer paren?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3052

Ok so as you see I broken out the compound statement, to be honest I find these compound if's unreadable and I can't for the life of me work out what case they are trying to handle. (I want to do this more)..

Please no one say doing it separately is slower! without measuring it..we are talking ns difference if anything at all.

I agree I truly believe we are mis assigning PointerOrReference/BinaryOperator this as I mentioned in
https://lists.llvm.org/pipermail/cfe-dev/2021-December/069486.html is a major issue and I think from time to time these rules try to correct that situation, in this case & is the logical operation on a reference.

3065

Man! you right...duh how did I miss that...

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

3052

I'm with you to split up these monsters!

[curdeius]

clang/lib/Format/TokenAnnotator.cpp

3054

Block, not line, comment

clang/unittests/Format/FormatTest.cpp

1947

How about pointers/references in the init? Also, please test sth else than auto, both in init and as the loop variable.

[MyDeveloperDay]

Address review comments

[MyDeveloperDay]

This fix has to go super specific to range for loops or we start manipulating pointer alignment all over the place in places perhaps we don't want to

[HazardyKnusperkeks]

You have reverted some of your changes. Including all the tests.

[MyDeveloperDay]

Fix the merge and add the tests back (which missed the patch)

[MyDeveloperDay]

In D115050#3172842, @HazardyKnusperkeks wrote:

You have reverted some of your changes. Including all the tests.

Yes oops!

[curdeius]

clang/unittests/Format/FormatTest.cpp

1947

I think that you're still missing non-auto variable declarations in init. Also, you can add a statement not being a variable declaration in the init.

[curdeius]

Requesting changes so that appears correctly in the review queue.

[MyDeveloperDay]

Add some more unit tests

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

1947

I think he meant something like
for (foo(); auto c : ...)

[curdeius]

clang/unittests/Format/FormatTest.cpp

1947

Indeed.
I'd like to see tests like these (some of them are already there):

for (auto x = 0; auto& c : {1, 2, 3})
for (auto x = 0; int& c : {1, 2, 3})
for (int x = 0; auto& c : {1, 2, 3})
for (int x = 0; int& c : {1, 2, 3})
for (f(); auto& c : {1, 2, 3})
for (f(); int& c : {1, 2, 3})

[MyDeveloperDay]

Add more unit tests

[curdeius]

LGTM! Thanks.

clang/lib/Format/TokenAnnotator.cpp

if (Right.is(TT_PointerOrReference)) {
     }
     // Add a space if the previous token is a pointer qualifier or the closing
     // parenthesis of __attribute__(()) expression and the style requires spaces
     // after pointer qualifiers.
     if ((Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_After ||
          Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_Both) &&
         (Left.is(TT_AttributeParen) || Left.canBePointerOrReferenceQualifier()))
       return true;
+    if (Left.Tok.isLiteral())
+      return true;
+    if (Left.Tok.is(tok::kw_auto))
+      return getTokenPointerOrReferenceAlignment(Right) !=
+             FormatStyle::PAS_Left;
     return (
-        Left.Tok.isLiteral() ||
         (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
          (getTokenPointerOrReferenceAlignment(Right) != FormatStyle::PAS_Left ||
           (Line.IsMultiVariableDeclStmt &&
            (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) ||
        (getTokenPointerOrReferenceAlignment(Left) != FormatStyle::PAS_Right &&
         !Line.IsMultiVariableDeclStmt)))
     return true;
   if (Left.is(TT_PointerOrReference)) {
     // Add a space if the next token is a pointer qualifier and the style
     // requires spaces before pointer qualifiers.
     if ((Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_Before ||
          Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_Both) &&
         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) &&
-            (getTokenPointerOrReferenceAlignment(Left) !=
-                 FormatStyle::PAS_Right &&
-             !Line.IsMultiVariableDeclStmt) &&
-            Left.Previous &&
-            !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon,
-                                    tok::l_square));
+    // & 1
+    if (Right.Tok.isLiteral())
+      return true;
+    // & // comment
+    if (Right.is(TT_BlockComment))
+      return true;
+    // foo() -> const Bar * override/final
+    if (Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
+        !Right.is(TT_StartOfName))
+      return true;
+    // & {
+    if (Right.is(tok::l_brace) && Right.is(BK_Block))
+      return true;
+    // for (auto a = 0, b = 0; const auto& c : {1, 2, 3})
+    if (Left.Previous && Left.Previous->is(tok::kw_auto) &&
+        Right.is(tok::identifier))
+      return getTokenPointerOrReferenceAlignment(Left) !=
+             FormatStyle::PAS_Right;
+
+    return (
+        !Right.isOneOf(TT_PointerOrReference, TT_ArraySubscriptLSquare,
+        (getTokenPointerOrReferenceAlignment(Left) != FormatStyle::PAS_Right &&
+        !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon, tok::l_square));
   }
   // Ensure right pointer alignment 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;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, SeparatePointerReferenceAlignment) {
   verifyFormat("int *f1(int *a, int &b, int &&c);", Style);
   verifyFormat("int &f2(int &&c, int *a, int &b);", Style);
   verifyFormat("int &&f3(int &b, int &&c, int *a);", Style);
   verifyFormat("int *f1(int &a) const &;", Style);
   verifyFormat("int *f1(int &a) const & = 0;", Style);
   verifyFormat("int *a = f1();", Style);
   verifyFormat("int &b = f2();", Style);
   verifyFormat("int &&c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const auto &c : {1, 2, 3})", Style);
 
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   verifyFormat("Const unsigned int *c;\n"
                "const unsigned int *d;\n"
                "Const unsigned int &e;\n"
                "const unsigned int &f;\n"
                "const unsigned    &&g;\n"
                "Const unsigned      h;",
                Style);
 
   Style.PointerAlignment = FormatStyle::PAS_Left;
   Style.ReferenceAlignment = FormatStyle::RAS_Pointer;
   verifyFormat("int* f1(int* a, int& b, int&& c);", Style);
   verifyFormat("int& f2(int&& c, int* a, int& b);", Style);
   verifyFormat("int&& f3(int& b, int&& c, int* a);", Style);
   verifyFormat("int* f1(int& a) const& = 0;", Style);
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int& b = f2();", Style);
   verifyFormat("int&& c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const auto& c : {1, 2, 3})", Style);
 
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   verifyFormat("Const unsigned int* c;\n"
                "const unsigned int* d;\n"
                "Const unsigned int& e;\n"
                "const unsigned int& f;\n"
                "const unsigned&&    g;\n"
                "Const unsigned      h;",
TEST_F(FormatTest, SeparatePointerReferenceAlignment) {
   Style.PointerAlignment = FormatStyle::PAS_Left;
   Style.ReferenceAlignment = FormatStyle::RAS_Middle;
   verifyFormat("int* f1(int* a, int & b, int && c);", Style);
   verifyFormat("int & f2(int && c, int* a, int & b);", Style);
   verifyFormat("int && f3(int & b, int && c, int* a);", Style);
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int & b = f2();", Style);
   verifyFormat("int && c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const auto & c : {1, 2, 3})", Style);
 
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   verifyFormat("Const unsigned int*  c;\n"
                "const unsigned int*  d;\n"
                "Const unsigned int & e;\n"
                "const unsigned int & f;\n"
                "const unsigned &&    g;\n"
                "Const unsigned       h;",