This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix aligning with linebreaks
ClosedPublic

Authored by HazardyKnusperkeks on Mar 8 2021, 12:47 PM.

Details

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";
}

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Mar 8 2021, 12:47 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2021, 12:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
curdeius requested changes to this revision.Mar 9 2021, 12:00 AM

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

clang/lib/Format/WhitespaceManager.cpp
280–281

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

332–340

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).

This revision now requires changes to proceed.Mar 9 2021, 12:00 AM
HazardyKnusperkeks marked 2 inline comments as done.Mar 9 2021, 1:00 PM
HazardyKnusperkeks added inline comments.
clang/lib/Format/WhitespaceManager.cpp
332–340

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.

HazardyKnusperkeks planned changes to this revision.Mar 9 2021, 1:00 PM
HazardyKnusperkeks marked an inline comment as done.
curdeius added inline comments.Mar 10 2021, 12:20 AM
clang/lib/Format/WhitespaceManager.cpp
332–340

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 marked 2 inline comments as done.
  • Addressed comments
  • Fixed handling of continued string literals when aligning
curdeius edited the summary of this revision. (Show Details)Mar 12 2021, 12:42 PM
curdeius accepted this revision.Mar 15 2021, 1:39 AM

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

This revision is now accepted and ready to land.Mar 15 2021, 1:39 AM
This revision was landed with ongoing or failed builds.Mar 28 2021, 7:27 AM
This revision was automatically updated to reflect the committed changes.

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;
}

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?

You are right.

The problem is in

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

line indentation. It is 6 instead of 4.

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.)

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));
}

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.

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.