[clang-format] fix indent in alignChainedConditionals

[krasimir]

Summary

Clang-format was indenting the lines following the ? in the added test
case by +5 instead of +4. This only happens in a very specific
situation, where the ? is followed by a multiline block comment, as in
the example. This fix addresses this without regressing any of the
existing tests.

From my understanding on the original patch https://github.com/llvm/llvm-project/commit/4db94094b469b4715d08ef37f1799bf3ea7ca8ea together with examining affected test cases,
I believe the intention of this section is to handle, e.g., the alignment of eeeeeeeeeeeeeeeeee and bbbbbbbbbbbbbbbb and 3333333333333333 in cases like this, where the previous toke is the ::

return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :
                           ccccccccccccc     ? dddddddddddddddddd :
                                               eeeeeeeeeeeeeeeeee) :
       bbbbbbbbbbbbbbbb ? 2222222222222222 :
                          3333333333333333;

The check for tok::question that I remove in this context seems superfluous in the first place: I believe currently we don't support a style where chained ternaries are formatted with newlines after the ?-s and alignment across the operands on a new line after the line ending with ?.

[sammccall]

Can't say I really understand this (either the before/after state).

It seems like we're trying to vertically-align a bunch of operands to a ?: chain, and this patch stops aligning the ones that follow a ? in certain cases. But I don't understand why this is correct, and what the point is of aligning (only) the ones following :.

Is there an intuitive explanation?

[krasimir]

I also have a limited understanding based on the original patch https://github.com/llvm/llvm-project/commit/4db94094b469b4715d08ef37f1799bf3ea7ca8ea together with examining affected test cases.
I believe the intention of this section is to handle, e.g., the alignment of eeeeeeeeeeeeeeeeee and bbbbbbbbbbbbbbbb and 3333333333333333 in cases like this, where the previous toke is the ::

return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :
                           ccccccccccccc     ? dddddddddddddddddd :
                                               eeeeeeeeeeeeeeeeee) :
       bbbbbbbbbbbbbbbb ? 2222222222222222 :
                          3333333333333333;

The check for tok::question that I remove in this context seems superfluous in the first place: I believe currently we don't support a style where chained ternaries are formatted with newlines after the ?-s and alignment across the operands on a new line after the line ending with ?.

clang/lib/Format/WhitespaceManager.cpp

AlignTokens(
                  ((C.Tok->is(tok::question) && !C.NewlinesBefore) ||
                   (C.Tok->is(tok::colon) && C.Tok->Next &&
                    (C.Tok->Next->FakeLParens.size() == 0 ||
                     C.Tok->Next->FakeLParens.back() != prec::Conditional)));
         },
         Changes, /*StartAt=*/0);
   } else {
     static auto AlignWrappedOperand = [](Change const &C) {
-      auto Previous = C.Tok->getPreviousNonComment(); // Previous;
+      FormatToken *Previous = C.Tok->getPreviousNonComment();
       return C.NewlinesBefore && Previous && Previous->is(TT_ConditionalExpr) &&
-             (Previous->is(tok::question) ||
-              (Previous->is(tok::colon) &&
-               (C.Tok->FakeLParens.size() == 0 ||
-                C.Tok->FakeLParens.back() != prec::Conditional)));
+               C.Tok->FakeLParens.back() != prec::Conditional));
     };
     // Ensure we keep alignment of wrapped operands with non-wrapped operands
     // Since we actually align the operators, the wrapped operands need the
     // extra offset to be properly aligned.
     for (Change &C : Changes) {
       if (AlignWrappedOperand(C))
         C.StartOfTokenColumn -= 2;
     }

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BreaksConditionalExpressions) {
   // FIXME: Aligning the question marks is weird given DontAlign.
   // Consider disabling this alignment in this case. Also check whether this
   // will render the adjustment from https://reviews.llvm.org/D82199
   // unnecessary.
   verifyFormat("int x = aaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa :\n"
                "    bbbb                ? cccccccccccccccccc :\n"
                "                          ddddd;\n",
                Style);
+
+  EXPECT_EQ(
+      "MMMMMMMMMMMMMMMMMMMMMMMMMMM = A ?\n"
+      "    /*\n"
+      "     */\n"
+      "    function() {\n"
+      "      try {\n"
+      "        return JJJJJJJJJJJJJJ(\n"
+      "            pppppppppppppppppppppppppppppppppppppppppppppppppp);\n"
+      "      }\n"
+      "    } :\n"
+      "    function() {};",
+      format(
+          "MMMMMMMMMMMMMMMMMMMMMMMMMMM = A ?\n"
+          "     /*\n"
+          "      */\n"
+          "     function() {\n"
+          "      try {\n"
+          "        return JJJJJJJJJJJJJJ(\n"
+          "            pppppppppppppppppppppppppppppppppppppppppppppppppp);\n"
+          "      }\n"
+          "    } :\n"
+          "    function() {};",
+          getGoogleStyle(FormatStyle::LK_JavaScript)));
 }
 
 TEST_F(FormatTest, BreaksConditionalExpressionsAfterOperator) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeTernaryOperators = false;
   Style.ColumnLimit = 70;
   verifyFormat(
       "aaaa(aaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaa ?\n"