This is an archive of the discontinued LLVM Phabricator instance.

[clang-Format] Fix indentation of member call after block
ClosedPublic

Authored by ank on Apr 17 2018, 6:30 AM.

Details

Summary

before patch:

echo "test() {([]() -> {int b = 32;return 3;}).as("");});" | clang-format -style=Google

test() {
  ([]() -> {
    int b = 32;
    return 3;
  })
      .as();
});

after patch:

echo "test() {([]() -> {int b = 32;return 3;}).as("");});" | clang-format -style=Google

test() {
  ([]() -> {
    int b = 32;
    return 3;
  }).as();
});

Diff Detail

Repository
rC Clang

Event Timeline

ank created this revision.Apr 17 2018, 6:30 AM
ank created this object with visibility "No One".
ank updated this revision to Diff 142763.Apr 17 2018, 6:36 AM
ank updated this revision to Diff 142764.Apr 17 2018, 6:40 AM
ank set the repository for this revision to rC Clang.
ank changed the visibility from "No One" to "Public (No Login Required)".
ank edited the summary of this revision. (Show Details)Apr 23 2018, 1:03 AM
klimek added inline comments.Apr 25 2018, 3:27 AM
unittests/Format/FormatTest.cpp
4425

What would be interesting is tests that:
a) have another value after the closing }; doesn't really make sense in this test, but usually these are in calls
f([]() { ... }, foo, bar).call(...)

b) make .as("") have paramters that go beyond the limit

c) add another chained call behind .as("").

ank added inline comments.May 29 2018, 7:08 AM
unittests/Format/FormatTest.cpp
4425

Hello, sorry for the late follow up on this!

Indeed an interesting thing to test, and so I did, the patterns you describe seems to give strange indentation as far back as release_36 and probably has always done so. The case I'm testing here worked in release_40 but stopped working somewhere before release_50

maybe fixing those other cases can be done in another patch

cheers,
Anders

ank added a comment.Jun 25 2018, 3:50 AM

Is there any chance to get this change or a similar one in so we get same behaviour as in release_40, even though it does not correct all of the problems?

klimek added inline comments.Jun 25 2018, 4:30 AM
unittests/Format/FormatTest.cpp
4425

I meant: please add these tests :)

ank added inline comments.Jun 25 2018, 5:53 AM
unittests/Format/FormatTest.cpp
4425

I need to clarify, those tests will not be correctly indented by this commit, I could add those tests but then they would verify a faulty behaviour, this is how they will be indented even with this patch applied:

// Dont break if only closing statements before member call
verifyFormat("test() {\n"
             "  ([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  }).foo();\n"
             "}");
verifyFormat("test() {\n"
             "  (\n"
             "      []() -> {\n"
             "        int b = 32;\n"
             "        return 3;\n"
             "      },\n"
             "      foo, bar)\n"
             "      .foo();\n"
             "}");
verifyFormat("test() {\n"
             "  ([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .foo()\n"
             "      .bar();\n"
             "}");
verifyFormat("test() {\n"
             "  ([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .foo(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
             "           \"bbbbbbbbbbbbbbbbbbbbbb\");\n"
             "}");

verifyFormat("test() {\n"
             "  foo([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  }).foo();\n"
             "}");
verifyFormat("test() {\n"
             "  foo(\n"
             "      []() -> {\n"
             "        int b = 32;\n"
             "        return 3;\n"
             "      },\n"
             "      foo, bar)\n"
             "      .as();\n"
             "}");
verifyFormat("test() {\n"
             "  foo([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .foo()\n"
             "      .bar();\n"
             "}");
verifyFormat("test() {\n"
             "  foo([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .as(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
             "          \"bbbbbbbbbbbbbbbbbbbbbb\");\n"
             "}");
klimek added inline comments.Jun 25 2018, 8:54 AM
unittests/Format/FormatTest.cpp
4425

I'm not sure we agree that the behavior is "faulty" - I do believe it was an intentional change :)
This is an indent of 4 from the start of the expression that call belongs to.
For example, in example (2), having the .foo() at the end of line after a potentially complex parameter strongly de-emphasizes the parameter.
In example (3), how would you want to layout a chain of calls?

ank updated this revision to Diff 152842.Jun 25 2018, 11:34 PM
ank added inline comments.
unittests/Format/FormatTest.cpp
4425

I see! I think your argument makes perfect sense, I think example 3 should be as is, considering that it is consistent with the other behaviour :) thanks for taking the time to explain!

klimek added inline comments.Jun 26 2018, 1:12 AM
unittests/Format/FormatTest.cpp
4449–4450

One more nit: I'd use a Style with a smaller column limit for these tests, makes them a lot more readable :)

4452

Are the ones below here testing anything that's not tested above?

ank updated this revision to Diff 152870.Jun 26 2018, 4:28 AM
ank added inline comments.
unittests/Format/FormatTest.cpp
4449–4450

makes sense, I'll do that.

4452

nope I'll remove them!

This revision is now accepted and ready to land.Jun 26 2018, 4:56 AM
ank added a comment.Jun 28 2018, 1:10 AM

awesome, I do not have merge rights so help with merging this would be greatly appreciated

ank added a comment.Aug 1 2018, 8:03 AM

Ping!

it would be awesome to get some help getting this merged since I do not have merge rights

klimek added a comment.Aug 1 2018, 8:43 AM

Sorry that I lost track of that, but feel free to ping once / week - unfortunately the patch doesn't apply cleanly, can you rebase?

danilaml added inline comments.
lib/Format/FormatToken.h
323

Looks like all other ifs in this file have space before (.

ank updated this revision to Diff 158737.EditedAug 2 2018, 5:19 AM

fix space in if (
and rebase

ank marked an inline comment as done.Aug 2 2018, 5:20 AM
ank added a comment.Aug 9 2018, 11:26 PM

Ping!

it would be awesome to get some help getting this merged since I do not have merge rights

Landed the patch as r342363.

This revision was automatically updated to reflect the committed changes.