Page MenuHomePhabricator

[clang-format] Add new LambdaBodyIndentation option
ClosedPublic

Authored by vlovich on May 18 2021, 11:25 AM.

Details

Summary

Currently the lambda body indents relative to where the lambda signature is located. This instead lets the user
choose to align the lambda body relative to the parent scope that contains the lambda declaration. Thus:

someFunction([] {
  lambdaBody();
});

will always have the same indentation of the body even when the lambda signature goes on a new line:

someFunction(
    [] {
  lambdaBody();
});

whereas before lambdaBody would be indented 6 spaces.

Diff Detail

Event Timeline

vlovich requested review of this revision.May 18 2021, 11:25 AM
vlovich created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 11:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vlovich added a comment.EditedMay 18 2021, 11:28 AM

I'm not 100% certain if the implementation is absolutely correct. For example, the #define A test case is broken (the closing brace is indented incorrectly). Additionally, the pop_back feels weird but seems to fix all the other cases to make the closing brace aligned properly. Maybe some/all of the implementation belongs in ContinuationIndenter.cpp instead but I couldn't really follow where the best place to do that might be. I can also just leave the preprocessor macro case as a known issue for now.

Maybe a bit more test cases with smaller lambdas? Or without the outer parenthesis?

clang/include/clang/Format/Format.h
2456

I think you should add examples, because for me it is not 100% what's the difference is.

2462

Here should also a comment be added.

Maybe a bit more test cases with smaller lambdas? Or without the outer parenthesis?

I'm not sure I understand this comment. Which test case are you referring to by "or without the outer parenthesis"?

clang/include/clang/Format/Format.h
2456

Good idea. Meant to do that & forgot. In terms of an example, do you find the test case or description in the phabricator message useful or are you looking for something else?

Maybe a bit more test cases with smaller lambdas? Or without the outer parenthesis?

I'm not sure I understand this comment. Which test case are you referring to by "or without the outer parenthesis"?

verifyFormat("test() {\n"
               "  ([]() -> {\n"
               "    int b = 32;\n"
               "    return 3;\n"
               "  }).foo();\n"
               "}",
               Style);

There you have parenthesis around the lambda, how about without?
Maybe just something like

std::sort(v.begin(), v.end(), [](const auto& lhs, const auto& rhs) { return lhs.Foo < rhs.Foo; });

verifyFormat("test() {\n"

"  ([]() -> {\n"
"    int b = 32;\n"
"    return 3;\n"
"  }).foo();\n"
"}",
Style);
There you have parenthesis around the lambda, how about without?
Maybe just something like

std::sort(v.begin(), v.end(), [](const auto& lhs, const auto& rhs) { return lhs.Foo < rhs.Foo; });

Ah. Ok. What I had done was run all the tests with this flag & note the ones where the formatting changed & copied those. The one without parenthesis didn't do anything interesting which is why I omitted it originally. I'll add it back + your sort example too. Thanks!

Review response

  • Add examples to documentation + links to KJ style guide.
  • Add better header doc
  • Add more test cases
  • Add release notes
  • Note current "problem" test with a TODO (corner case that's not critical for KJ).
vlovich marked 2 inline comments as done.May 20 2021, 9:48 PM

Is `Signature``` effectively as is?

Could you clang-format the tests please so I can more easily read them.

clang/docs/ClangFormatStyleOptions.rst
2826

Did you generate this by hand or using the clang/doc/tools/dump_style.py?

Could you clang-format the tests please so I can more easily read them.

Sure. I was just taking my queue from the rest of the file which appeared to intentionally not clang-format the test strings.

clang/docs/ClangFormatStyleOptions.rst
2826

By hand. dump_format_style generates other changes (although I wasn't aware of it at the time). I'll regenerate just this section using that tool.

Is `Signature``` effectively as is?

Yes. I noted that it's the default behavior. Please let me know if I can clarify the documentation wording somehow (or if you have preferred wording you want me to use)

Regen documentation & clang-format test cases.

I personally like to keep clang-format and clang-format tests clang formatted all the time so we have a reasonable set of "clang-format-clean" code we can sanity check any change against, the more of LLVM that can be clean the better our testing can be.

clang/docs/ClangFormatStyleOptions.rst
2826

it shouldn't, that probably means someone has made a change without keeping them consistent..I'll take a look at that.

vlovich marked an inline comment as done.May 27 2021, 12:58 PM

I think all review comments have been addressed. Please let me know if there's anything else blocking merge.

LGTM, but please wait for more responses.

This revision is now accepted and ready to land.May 28 2021, 12:25 PM

LGTM, but please wait for more responses.

No one objected, so I declare it ready to commit. Do you need some one to do it? If yes please post name and email for the commit.

vlovich added a comment.EditedJun 16 2021, 1:55 PM

I don't think I have permissions. Happy to do it if I'm given permissions (I'm assuming the instructions are the general LLVM ones). Otherwise:

Name: Vitali Lovich
E-mail: vlovich@gmail.com

I don't think I have permissions. Happy to do it if I'm given permissions (I'm assuming the instructions are the general LLVM ones). Otherwise:

Name: Vitali Lovich
E-mail: vlovich@gmail.com

You can apply for commit access, yes that's for whole of LLVM, but for me it was no problem to get that right.

I think you can go ahead & commit it for me. I don't know when I'll get commit access.

This revision was automatically updated to reflect the committed changes.
clang/include/clang/Format/Format.h
2482

Here is a ` missing, I'll fix it.