[clang-format] Fix aligning with linebreaks

[HazardyKnusperkeks]

Summary

Breaking a string literal or a function calls arguments with
AlignConsecutiveDeclarations or AlignConsecutiveAssignments did misalign
the continued line. E.g.:

void foo() {
  int myVar = 5;
  double x  = 3.14;
  auto str  = "Hello"
            "World";
}

or

void foo() {
  int    myVar = 5;
  double x = 3.14;
  auto   str = "Hello"
             "World";
}

[curdeius]

This looks really good. Just a few rather minor remarks.

clang/lib/Format/WhitespaceManager.cpp

280–289

Nit: I'd break the line before the new phrase.

331–360

Would it be possible to break up this condition and name it (or name its parts)? It's getting hard to follow.
Suggestion according to my understanding, which might be wrong.

clang/unittests/Format/FormatTest.cpp

14305–14311

Nice tests. I'd like to see however the behaviour when there's another assignment/declaration after a multi-line string, maybe even mixing multi-line strings and multi-line function calls. You already do test multiple multi-line function calls (with newEntry and newEntry2).

[HazardyKnusperkeks]

clang/lib/Format/WhitespaceManager.cpp

331–360

Can do, but then all those are always checked, there is no short circuit anymore. But I really get your point.

How about a lambda with different returns (and comments), that way we would still short circuit. Or some thing like

bool AddShift = /* checks #1 */;
AddShift = AddShift || /* checks #2 */;
...
AddShift = AddShoft || /* checks #n */;

if (AddShift)
  Changes[i].Spaces += Shift;

clang/unittests/Format/FormatTest.cpp

14305–14311

Mixing can be done, but I really failed to come up with a function call scenario in the short version. That's why I took my (slightly modified) real life example where I hit that bug.

Also: Nice comment, I found another bug. It seems that the alignment stops after the broken string literal, I have no fix yet, but an idea. Will reupload when done.

[curdeius]

clang/lib/Format/WhitespaceManager.cpp

331–360

Good point on the short circuiting. A lambda may be a good solution here. But the bool AddShift ... that you suggested above (with comments too) is ok for me as well. I let you choose.

[HazardyKnusperkeks]

• Addressed comments
• Fixed handling of continued string literals when aligning

[curdeius]

LGTM. Let some time to others to have a look before landing.

[baramin]

We have a regression after the fix:
CPP-25899 CLion formatting does not match what clang-format produces

if

AlignConsecutiveAssignments: Consecutive

Additional two spaces before = were added to indentation while formatting

void SomeFunc()
{
    using DcgmNs::Timelib::FromLegacyTimestamp;
    using DcgmNs::Timelib::ToLegacyTimestamp;
    using DcgmNs::Utils::GetMaxAge;
    using namespace std::chrono;
    newWatcher.maxAgeUsec   = ToLegacyTimestamp(GetMaxAge(
          FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.isSubscribed = subscribeForUpdates ? 1 : 0;
}

[MyDeveloperDay]

In D98214#2900371, @baramin wrote:

We have a regression after the fix:
CPP-25899 CLion formatting does not match what clang-format produces

if

AlignConsecutiveAssignments: Consecutive

Additional two spaces before = were added to indentation while formatting

void SomeFunc()
{
    using DcgmNs::Timelib::FromLegacyTimestamp;
    using DcgmNs::Timelib::ToLegacyTimestamp;
    using DcgmNs::Utils::GetMaxAge;
    using namespace std::chrono;
    newWatcher.maxAgeUsec   = ToLegacyTimestamp(GetMaxAge(
          FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.isSubscribed = subscribeForUpdates ? 1 : 0;
}

At first glance this looks correct to me, newWatcher.maxAgeUsec = and newWatcher.isSubscribed = are considered consecutive. Am I missing something?

[baramin]

You are right.

The problem is in

FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));

line indentation. It is 6 instead of 4.

[HazardyKnusperkeks]

In D98214#2900803, @baramin wrote:

You are right.

The problem is in

FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));

line indentation. It is 6 instead of 4.

What you call a regression I call a fix. :)
I can not see the .clang-format on you linked issue.

If we agree on that it is a bug, I will try to fix it, but that decision have others to do. (And then I will need the .clang-format.)

[baramin]

This is true.
.clang-format:

Language:        Cpp
AlignConsecutiveAssignments: Consecutive
BinPackArguments: false
BinPackParameters: false
ColumnLimit:     120
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
IndentWidth:     4
TabWidth:        4
UseCRLF:         false
UseTab:          Never

Format, that looks like a regression for me:
Now:

void SomeFunc() {
    newWatcher.maxAgeUsec = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.maxAge     = ToLegacyTimestamp(GetMaxAge(
            FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.max        = ToLegacyTimestamp(GetMaxAge(
               FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
}

Before:

void SomeFunc() {
    newWatcher.maxAgeUsec = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.maxAge     = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.max        = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
}

[HazardyKnusperkeks]

In D98214#2901304, @baramin wrote:

This is true.
.clang-format:

Language:        Cpp
AlignConsecutiveAssignments: Consecutive
BinPackArguments: false
BinPackParameters: false
ColumnLimit:     120
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
IndentWidth:     4
TabWidth:        4
UseCRLF:         false
UseTab:          Never

Format, that looks like a regression for me:
Now:

void SomeFunc() {
    newWatcher.maxAgeUsec = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.maxAge     = ToLegacyTimestamp(GetMaxAge(
            FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.max        = ToLegacyTimestamp(GetMaxAge(
               FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
}

Before:

void SomeFunc() {
    newWatcher.maxAgeUsec = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.maxAge     = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.max        = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
}

Yeah okay, that looks wrong. I will take a look at it.

[HazardyKnusperkeks]

In D98214#2901304, @baramin wrote:

This is true.
.clang-format:

Language:        Cpp
AlignConsecutiveAssignments: Consecutive
BinPackArguments: false
BinPackParameters: false
ColumnLimit:     120
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
IndentWidth:     4
TabWidth:        4
UseCRLF:         false
UseTab:          Never

Format, that looks like a regression for me:
Now:

void SomeFunc() {
    newWatcher.maxAgeUsec = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.maxAge     = ToLegacyTimestamp(GetMaxAge(
            FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.max        = ToLegacyTimestamp(GetMaxAge(
               FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
}

Before:

void SomeFunc() {
    newWatcher.maxAgeUsec = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.maxAge     = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
    newWatcher.max        = ToLegacyTimestamp(GetMaxAge(
        FromLegacyTimestamp<milliseconds>(monitorFrequencyUsec), seconds(std::uint64_t(maxSampleAge)), maxKeepSamples));
}

The fix is in D106773, please have a look at it.

clang/lib/Format/WhitespaceManager.cpp

AlignTokenSequence(unsigned Start, unsigned End, unsigned Column, F &&Matches,
   // We only run the "Matches" function on tokens from the outer-most scope.
   // However, we do need to pay special attention to one class of tokens
   // that are not in the outer-most scope, and that is function parameters
   // which are split across multiple lines, as illustrated by this example:
   //   double a(int x);
   //   int    b(int  y,
   //          double z);
   // In the above example, we need to take special care to ensure that
   // 'double z' is indented along with it's owning function 'b'.
+  // The same holds for calling a function:
+  //   double a = foo(x);
+  //   int    b = bar(foo(y),
+  //            foor(z));
+  // Similar for broken string literals:
+  //   double x = 3.14;
+  //   auto s   = "Hello"
+  //          "World";
   // Special handling is required for 'nested' ternary operators.
   SmallVector<unsigned, 16> ScopeStack;
 
   for (unsigned i = Start; i != End; ++i) {
     if (ScopeStack.size() != 0 &&
         Changes[i].indentAndNestingLevel() <
             Changes[ScopeStack.back()].indentAndNestingLevel())
       ScopeStack.pop_back();
 
     // Compare current token to previous non-comment token to ensure whether
     // it is in a deeper scope or not.
     unsigned PreviousNonComment = i - 1;
     while (PreviousNonComment > Start &&
            Changes[PreviousNonComment].Tok->is(tok::comment))
       PreviousNonComment--;
     if (i != Start && Changes[i].indentAndNestingLevel() >
                           Changes[PreviousNonComment].indentAndNestingLevel())
       ScopeStack.push_back(i);
 
     bool InsideNestedScope = ScopeStack.size() != 0;
+    bool ContinuedStringLiteral = i > Start &&
+                                  Changes[i].Tok->is(tok::string_literal) &&
+                                  Changes[i - 1].Tok->is(tok::string_literal);
+    bool SkipMatchCheck = InsideNestedScope || ContinuedStringLiteral;
 
-    if (Changes[i].NewlinesBefore > 0 && !InsideNestedScope) {
+    if (Changes[i].NewlinesBefore > 0 && !SkipMatchCheck) {
       Shift = 0;
       FoundMatchOnLine = false;
     }
 
     // If this is the first matching token to be aligned, remember by how many
     // spaces it has to be shifted, so the rest of the changes on the line are
     // shifted by the same amount
-    if (!FoundMatchOnLine && !InsideNestedScope && Matches(Changes[i])) {
+    if (!FoundMatchOnLine && !SkipMatchCheck && Matches(Changes[i])) {
       FoundMatchOnLine = true;
       Shift = Column - Changes[i].StartOfTokenColumn;
       Changes[i].Spaces += Shift;
     }
 
     // This is for function parameters that are split across multiple lines,
     // as mentioned in the ScopeStack comment.
     if (InsideNestedScope && Changes[i].NewlinesBefore > 0) {
       unsigned ScopeStart = ScopeStack.back();
-      if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName) ||
-          (ScopeStart > Start + 1 &&
-           Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)) ||
-          Changes[i].Tok->is(TT_ConditionalExpr) ||
-          (Changes[i].Tok->Previous &&
-           Changes[i].Tok->Previous->is(TT_ConditionalExpr)))
+      auto ShouldShiftBeAdded = [&] {
+        // Function declaration
+        if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName))
+          return true;
+
+        // Continued function declaration
+        if (ScopeStart > Start + 1 &&
+            Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName))
+          return true;
+
+        // Continued function call
+        if (ScopeStart > Start + 1 &&
+            Changes[ScopeStart - 2].Tok->is(tok::identifier) &&
+            Changes[ScopeStart - 1].Tok->is(tok::l_paren))
+          return true;
+
+        // Ternary operator
+        if (Changes[i].Tok->is(TT_ConditionalExpr))
+          return true;
+
+        // Continued ternary operator
+        if (Changes[i].Tok->Previous &&
+            Changes[i].Tok->Previous->is(TT_ConditionalExpr))
+          return true;
+
+        return false;
+      };
+
+      if (ShouldShiftBeAdded())
         Changes[i].Spaces += Shift;
     }
 
+    if (ContinuedStringLiteral)
+      Changes[i].Spaces += Shift;
+
     assert(Shift >= 0);
     Changes[i].StartOfTokenColumn += Shift;
     if (i + 1 != Changes.size())
       Changes[i + 1].PreviousEndOfTokenColumn += Shift;
   }
 }
 
 // Walk through a subset of the changes, starting at StartAt, and find
if (Changes[i].NewlinesBefore != 0) {
           !(LineIsComment &&
             ((ACS == FormatStyle::ACS_AcrossComments) ||
              (ACS == FormatStyle::ACS_AcrossEmptyLinesAndComments)));
 
       if (EmptyLineBreak || NoMatchBreak)
         AlignCurrentSequence();
 
       // A new line starts, re-initialize line status tracking bools.
+      // Keep the match state if a string literal is continued on this line.
+      if (i == 0 || !Changes[i].Tok->is(tok::string_literal) ||
+          !Changes[i - 1].Tok->is(tok::string_literal))
         FoundMatchOnLine = false;
       LineIsComment = true;
     }
 
     if (!Changes[i].Tok->is(tok::comment)) {
       LineIsComment = false;
     }
 
     if (Changes[i].Tok->is(tok::comma)) {

clang/unittests/Format/FormatTest.cpp

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;",
                Alignment);
 }
 
+TEST_F(FormatTest, AlignWithLineBreaks) {
+  auto Style = getLLVMStyleWithColumns(120);
+
+  EXPECT_EQ(Style.AlignConsecutiveAssignments, FormatStyle::ACS_None);
+  EXPECT_EQ(Style.AlignConsecutiveDeclarations, FormatStyle::ACS_None);
+  verifyFormat("void foo() {\n"
+               "  int myVar = 5;\n"
+               "  double x = 3.14;\n"
+               "  auto str = \"Hello \"\n"
+               "             \"World\";\n"
+               "  auto s = \"Hello \"\n"
+               "           \"Again\";\n"
+               "}",
+               Style);
+
+  // clang-format off
+  verifyFormat("void foo() {\n"
+               "  const int capacityBefore = Entries.capacity();\n"
+               "  const auto newEntry = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                            std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "  const X newEntry2 = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                          std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "}",
+               Style);
+  // clang-format on
+
+  Style.AlignConsecutiveAssignments = FormatStyle::ACS_Consecutive;
+  verifyFormat("void foo() {\n"
+               "  int myVar = 5;\n"
+               "  double x  = 3.14;\n"
+               "  auto str  = \"Hello \"\n"
+               "              \"World\";\n"
+               "  auto s    = \"Hello \"\n"
+               "              \"Again\";\n"
+               "}",
+               Style);
+
+  // clang-format off
+  verifyFormat("void foo() {\n"
+               "  const int capacityBefore = Entries.capacity();\n"
+               "  const auto newEntry      = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                                 std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "  const X newEntry2        = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                                 std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "}",
+               Style);
+  // clang-format on
+
+  Style.AlignConsecutiveAssignments = FormatStyle::ACS_None;
+  Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
+  verifyFormat("void foo() {\n"
+               "  int    myVar = 5;\n"
+               "  double x = 3.14;\n"
+               "  auto   str = \"Hello \"\n"
+               "               \"World\";\n"
+               "  auto   s = \"Hello \"\n"
+               "             \"Again\";\n"
+               "}",
+               Style);
+
+  // clang-format off
+  verifyFormat("void foo() {\n"
+               "  const int  capacityBefore = Entries.capacity();\n"
+               "  const auto newEntry = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                            std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "  const X    newEntry2 = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                             std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "}",
+               Style);
+  // clang-format on
+
+  Style.AlignConsecutiveAssignments = FormatStyle::ACS_Consecutive;
+  Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
+
+  verifyFormat("void foo() {\n"
+               "  int    myVar = 5;\n"
+               "  double x     = 3.14;\n"
+               "  auto   str   = \"Hello \"\n"
+               "                 \"World\";\n"
+               "  auto   s     = \"Hello \"\n"
+               "                 \"Again\";\n"
+               "}",
+               Style);
+
+  // clang-format off
+  verifyFormat("void foo() {\n"
+               "  const int  capacityBefore = Entries.capacity();\n"
+               "  const auto newEntry       = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                                  std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "  const X    newEntry2      = Entries.emplaceHint(std::piecewise_construct, std::forward_as_tuple(uniqueId),\n"
+               "                                                  std::forward_as_tuple(id, uniqueId, name, threadCreation));\n"
+               "}",
+               Style);
+  // clang-format on
+}
+
 TEST_F(FormatTest, LinuxBraceBreaking) {
   FormatStyle LinuxBraceStyle = getLLVMStyle();
   LinuxBraceStyle.BreakBeforeBraces = FormatStyle::BS_Linux;
   verifyFormat("namespace a\n"
                "{\n"
                "class A\n"
                "{\n"
                "  void f()\n"