Page MenuHomePhabricator

[clang-format] Add InsertBraces option
AcceptedPublic

Authored by tiagoma on Jan 21 2021, 1:22 PM.

Details

Summary

InsertBraces will insert braces for single line control flow statements (currently if/else/for).
It currently accepts the following options:

  • Never - Keeps the current behavior and never add braces (also default value)
  • Always - Always add braces to single lines after the control flow statement
  • WrapLikely - If the line after the control flow statement is not braced and likely to wrap, braces will be added

Diff Detail

Event Timeline

tiagoma requested review of this revision.Jan 21 2021, 1:22 PM
tiagoma created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 1:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tiagoma added a reviewer: curdeius.

There's a clang-tidy check for it.
And clang-format should not, IMO, add not remove tokens but only handle whitespace.

Some background might be useful. I work at Microsoft (Office to be precise). We have our own fork of LLVM internally and I am starting to upstream some of the changes in our fork.

There's a clang-tidy check for it.

Yes - readability-braces-around-statements. I would argue that clang-tidy is a heavyweight tool for this.

And clang-format should not, IMO, add not remove tokens but only handle whitespace.

I don't think token operations are novel, see the InsertTrailingCommas, JavaScriptQuotes, SortUsingDeclarations, FixNamespaceComments options. Also, keep in mind that this option is fairly self contained (it happens before the Formatter pass) and it is off by default.

True, I was aware of the presence of some of these options, thank you for indicating others. I'm not yet entirely convinced, especially that clang-tidy behaviour would be possibly different.

Anyway, a few comments just from the first glance.

clang/docs/ClangFormatStyleOptions.rst
2569

Shouldn't it be consistent with clang-tidy? So instead of an enum, this option might take a number of lines that will trigger brace insertion? None may be sth like -1. "Likely" is very vague.

clang/lib/Format/Format.cpp
1739

Nit: put full stop at the end of the comments.

1753

And make the comments full phrases, starting with a capital letter.

tiagoma added inline comments.Jan 22 2021, 9:56 AM
clang/docs/ClangFormatStyleOptions.rst
2569

I am not sure this is possible. clang-tidy assumes the code is already formatted, we cannot assume that in clang-format. This pass runs before the formatter pass, so basing the behavior on line count would lead to different behavior across runs. For example:

Assuming we can set this option to 2 lines and we have the following code:

if (condition)
	very_long_line_that_will_wrap(arg, arg, arg ,arg)

first run:

if (condition)
	very_long_line_that_will_wrap(
		arg, arg, arg ,arg)

second run:

if (condition) {
	very_long_line_that_will_wrap(
		arg, arg, arg ,arg)}
}

Likely is indeed vague by design. We "guess" that it will wrap in the formatter pass based on the current column value. See Format.cpp @ 1707. What might happen is that the line is incorrectly indentented and the wrapping never actually happens.

lebedev.ri retitled this revision from Add InsertBraces option to [clang-format] Add InsertBraces option.Jan 22 2021, 9:58 AM
tiagoma updated this revision to Diff 318563.Jan 22 2021, 10:09 AM

Update comments

tiagoma marked 2 inline comments as done.Jan 22 2021, 10:09 AM

I think this is one of those reviews that ultimately I think would be useful if we could ensure it works 100% correctly, but I think it goes against the original ethos of clang-format and I think if @djasper or @klimek the original authors were here they'd probably push back.

This reminds me of my own work for a East/West fixer, it proved to be a bit too controversial for some, despite being off by default. but I also agree clang-format has been inserting and moving code around for some time, so its no longer just a whitespace manipulation tool.

I do like this, and I agree clang-tidy is often too heavy weight ( For my source tree which is millions of line of code running clang-tidy over the whole thing is just not possible). I personally run "clang-format -n" over my entire tree every night to sanity check it, this feature would allow me to enforce the use of {} without which I've found over the years is often a source of bugs.

lets see if others have an opinion

MyDeveloperDay added inline comments.Jan 22 2021, 3:55 PM
clang/unittests/Format/FormatTest.cpp
19084

can you use verifyFormat() instead of EXPECT_EQ?

19159

are you testing do/while?

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

Hmm, interesting idea. Do you have anything more precise in mind? Would it be an "augmented" clang-format? I.e. it will have all its options and some additional ones? Or rather more independent tool? Or clang-format's experimental field?

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

I don't think this should be done. These kinds of things should be in clang-format. One of the advantages of this and east/west const being in clang-format is that editors, CI systems and other tools have clang-format support. They would be unlikely to get support for a new tool. There are plenty of clang tools which exist but which don't get enough attention to get support in editors CI tools etc.

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

I don't think this should be done. These kinds of things should be in clang-format. One of the advantages of this and east/west const being in clang-format is that editors, CI systems and other tools have clang-format support. They would be unlikely to get support for a new tool. There are plenty of clang tools which exist but which don't get enough attention to get support in editors CI tools etc.

Not saying that I'm in favour of creating another tool, but...
I believe that such a tool, if it were pretty much a drop-in replacement of clang-format, it could profit from the current tooling support.
Why? Because most of them let you define the clang-format binary path.

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

I don't think this should be done. These kinds of things should be in clang-format. One of the advantages of this and east/west const being in clang-format is that editors, CI systems and other tools have clang-format support. They would be unlikely to get support for a new tool. There are plenty of clang tools which exist but which don't get enough attention to get support in editors CI tools etc.

Not saying that I'm in favour of creating another tool, but...
I believe that such a tool, if it were pretty much a drop-in replacement of clang-format, it could profit from the current tooling support.

If it's a drop in replacement (does everything clang-format does and more), what's the benefit for that cost?

If it's a drop in replacement (does everything clang-format does and more), what's the benefit for that cost?

I'm in agreement here, It was a suggestion just to appease those who don't like clang-format doing such things, but still allowing us to add code changing capabilities. (which I don't understand why that isn't controlled simply by turning the capability off.)

MyDeveloperDay added inline comments.Feb 2 2021, 8:24 AM
clang/unittests/Format/FormatTest.cpp
19067

can these all be verifyFormats

19159

whilst people discuss the ethics of modifying the code ;-)

Can you add some comment based examples

if (condition) // my test
      you_do_you();

if (condition)
      you_do_you(); // my test
MyDeveloperDay added inline comments.Feb 2 2021, 8:27 AM
clang/unittests/Format/FormatTest.cpp
19159

bonus points..

if /*condition*/ (condition) /*condition*/
/*condition*/      you_do_you(); /*condition*/

I'm just going to say this here, but LLVM doesn't like the use of braces on single lines (I don't actually like that myself, but I go with the convention when I remember),

but this is followed only if the reviewer catches it, but by and large its super easy for them to get missed, it took me seconds to find an example..

I would love to see the end for the need for the "elide braces" comment in code reviews, by having an ability to "RemoveBraces"

i.e. this feature should be able to remove the {} in this case

// Defaults that differ when not C++.
if (Language == FormatStyle::LK_TableGen) {
  LLVMStyle.SpacesInContainerLiterals = false;
}
njames93 added inline comments.
clang/unittests/Format/FormatTest.cpp
19159

Should also add test for chained conditionals just to make sure the semantics of the code doesn't change.

if (A)
  if (B)
    callAB();
  else
    callA();
else if (B)
  callB();
else
  call();
tiagoma updated this revision to Diff 321611.Feb 4 2021, 4:28 PM

Add more tests

tiagoma marked 5 inline comments as done.Feb 4 2021, 4:35 PM
tiagoma added inline comments.
clang/unittests/Format/FormatTest.cpp
19159

do/while are not supported in it's current form. We would need to change the logic to add more state. I can have a look at it after this patch is accepted.

19159

This specific test will fail by itself, ie:

StringRef Test = "if /*condition*/ (condition)  /*condition*/\n"
                 "  /*condition*/ you_do_you(); /*condition*/";

verifyFormat(Test);

AFAICT it is because test::messUp causes the 2nd and 3rd comment to stay on the same line. I added a variation of the test.

MyDeveloperDay added inline comments.Feb 5 2021, 3:35 AM
clang/unittests/Format/FormatTest.cpp
19159

The problem with this statement is as soon as you commit, we'd get that defect raised, that is ok its just are you going to stay around long enough to finish it completely? ;-) if not then this puts burden on those who hang out here all the time.

Ideally its probably worth following through with a complete implementation before landing (or as best you can), there is no rush right?

tiagoma updated this revision to Diff 322240.Feb 8 2021, 4:33 PM

Add support for do/while

tiagoma marked an inline comment as done.Feb 9 2021, 10:10 AM
MyDeveloperDay accepted this revision.Feb 13 2021, 7:09 AM

LGTM, we need to run this on a large code base to ensure it doesn't make mistakes in real code, but I think this looks good.

This revision is now accepted and ready to land.Feb 13 2021, 7:09 AM