[clang-format] use spaces for alignment of binary/ternary expressions with UT_AlignWithSpaces

[fickert]

Summary

Use spaces to align binary and ternary expressions when using AlignOperands and UT_AlignWithSpaces.

[fickert]

This fixes an oversight in the new UT_AlignWithSpaces option (see D75034), which did not correctly identify the alignment of binary/ternary expressions.

[curdeius]

clang/lib/Format/ContinuationIndenter.cpp

1366–1371

You could factor out the part of the condition that doesn't concern AlignOperands to get rid of the repetition wrt. the previous if.

[fickert]

Resolved repetitive conditions in the two consecutive ifs and added a small comment.

[curdeius]

LGTM. Thank you for the patch!

[fickert]

Thanks! I don't have push permissions to the repository so I cannot submit the commit myself (Maximilian Fickert <[e-mail]>).

[Typz]

Could this also be causing https://bugs.llvm.org/show_bug.cgi?id=33896 ?

I'll try to see if changes something in that case...

[Typz]

In D85600#2209545, @Typz wrote:

Could this also be causing https://bugs.llvm.org/show_bug.cgi?id=33896 ?

I'll try to see if changes something in that case...

Actually, I can confirm this is unrelated.

clang/lib/Format/ContinuationIndenter.cpp

1360

bad indent here : should be space-indented.

[fickert]

Yes I think it's unrelated, though this patch should fix the alignment in that example when using the UT_AlignWithSpaces option.

[fickert]

clang/lib/Format/ContinuationIndenter.cpp

1360

Can you clarify? This is formatted with the LLVM clang-format.

[Typz]

clang/lib/Format/ContinuationIndenter.cpp

1360

Phabricator shows some kind of >> symbol at the beginning of this line, which i interpreted as a TAB character, hence my comment.
But indeed no issue after looking at the raw file, sorry about this.

[sylvestre.ledru]

I am not 100% that it is thanks to this patch but reformatting Firefox code with clang-format 11 significantly improves the readability of the usage
of ternary operators. See: https://phabricator.services.mozilla.com/D90795

Thanks for the change!

clang/lib/Format/ContinuationIndenter.cpp

if (!Current.isTrailingComment() &&
          (Style.Language != FormatStyle::LK_Java && *I > 0)) &&
         (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign ||
          *I != prec::Comma || Current.NestingLevel == 0)) {
       NewParenState.Indent =
           std::max(std::max(State.Column, NewParenState.Indent),
                    State.Stack.back().LastSpace);
     }
 
-    // If BreakBeforeBinaryOperators is set, un-indent a bit to account for
-    // the operator and keep the operands aligned
-    if (Style.AlignOperands == FormatStyle::OAS_AlignAfterOperator &&
-        Previous &&
+    if (Previous &&
         (Previous->getPrecedence() == prec::Assignment ||
          Previous->is(tok::kw_return) ||
          (*I == prec::Conditional && Previous->is(tok::question) &&
           Previous->is(TT_ConditionalExpr))) &&
-        !Newline)
+        !Newline) {
+      // If BreakBeforeBinaryOperators is set, un-indent a bit to account for
+      // the operator and keep the operands aligned
+      if (Style.AlignOperands == FormatStyle::OAS_AlignAfterOperator)
         NewParenState.UnindentOperator = true;
+      // Mark indentation as alignment if the expression is aligned.
+      if (Style.AlignOperands != FormatStyle::OAS_DontAlign)
+        NewParenState.IsAligned = true;
+    }
 
     // Do not indent relative to the fake parentheses inserted for "." or "->".
     // This is a special case to make the following to statements consistent:
     //   OuterFunction(InnerFunctionCall( // break
     //       ParameterToInnerFunction));
     //   OuterFunction(SomeObject.InnerFunctionCall( // break
     //       ParameterToInnerFunction));
     if (*I > prec::Unknown)
       NewParenState.LastSpace = std::max(NewParenState.LastSpace, State.Column);
     if (*I != prec::Conditional && !Current.is(TT_UnaryOperator) &&
         Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign)
       NewParenState.StartOfFunctionCall = State.Column;
 
     // Indent conditional expressions, unless they are chained "else-if"
     // conditionals. Never indent expression where the 'operator' is ',', ';' or

clang/unittests/Format/FormatTest.cpp

verifyFormat("class Assign {\n"
                "\tvoid f() {\n"
                "\t\tint         x      = 123;\n"
                "\t\tint         random = 4;\n"
                "\t\tstd::string alphabet =\n"
                "\t\t\t\"abcdefghijklmnopqrstuvwxyz\";\n"
                "\t}\n"
                "};",
                Tab);
+  Tab.AlignOperands = FormatStyle::OAS_Align;
+  verifyFormat("int aaaaaaaaaa = bbbbbbbbbbbbbbbbbbbb +\n"
+               "                 cccccccccccccccccccc;",
+               Tab);
+  // no alignment
+  verifyFormat("int aaaaaaaaaa =\n"
+               "\tbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;",
+               Tab);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 111111111111111\n"
+               "       : bbbbbbbbbbbbbb ? 222222222222222\n"
+               "                        : 333333333333333;",
+               Tab);
+  Tab.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Tab.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+  verifyFormat("int aaaaaaaaaa = bbbbbbbbbbbbbbbbbbbb\n"
+               "               + cccccccccccccccccccc;",
+               Tab);
 }
 
 TEST_F(FormatTest, ZeroTabWidth) {
   FormatStyle Tab = getLLVMStyleWithColumns(42);
   Tab.IndentWidth = 8;
   Tab.UseTab = FormatStyle::UT_Never;
   Tab.TabWidth = 0;
   EXPECT_EQ("void a(){\n"