See the LLVM style examples. This option can be expanded/configured to support other styles.
Details
Diff Detail
Event Timeline
Could you have a look at preceding reviews and see if there wasn't a similar patch before?
I think that this option is a bit too limited.
Only removing braces doesn't seem enough.
Also, one should probably be able to decide when to add/remove them by e.g. setting the number of lines in what's considered short blocks.
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
3405 | I don't think it's a good idea to make this option dependent on a particular style. Even if it works for LLVM style only for the moment. | |
3426 | I'm not sure if the braces on the right should be removed in the for loop. |
There is D95168.
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
490 | This is very confusing a variable with the same name as the function. | |
2207 | constexpr or configurable? | |
2433 | I think it is worth to create a RAII type for that. | |
3043 | I would like it better if you insert both arguments. | |
clang/lib/Format/UnwrappedLineParser.h | ||
199 | I find it nicely if the order of functions in the cpp is the same as in the hpp. | |
200 |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
3426 |
This is the LLVM style. See also line 23086 in FormatTest.cpp below.
If you have more than one statement inside a control statement block, the braces must stay. |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2207 | It will be configurable when we expand removing braces to other styles. | |
2433 | Can you explain why an RAII type would be beneficial here? | |
3043 | I'm the opposite, and parseBlock()is a good example. However, I restored the previously non-default argument. | |
clang/lib/Format/UnwrappedLineParser.h | ||
199 | Me too although some of the functions (e.g. parseCSharpGenericTypeConstraint and parseCSharpAttribute) were already out of order. |
I conversed with @MyDeveloperDay in D95168. In fact, that was one of the main reasons that I put in extra effort to get this patch done.
I think that this option is a bit too limited.
Only removing braces doesn't seem enough.
I wanted to implement as many features as possible using the limited parser of clang-format, and the LLVM style would be a very high bar to clear. Only focusing on LLVM now will actually make this option less limited if and when we are ready to support other styles. It would be mostly configuring/packaging what have already worked for LLVM.
Also, one should probably be able to decide when to add/remove them by e.g. setting the number of lines in what's considered short blocks.
Agreed, but if clang-format can't do insertion well independent of removal and vice versa, we probably will never get there.
So far, this patch has successfully applied to clang/lib/Format, i.e., it builds successfully and passes FormatTests.
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2433 | You have a lot of push/pop pairs. There is the risk someone (in the future) may misuse it. |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2378 | Why null that? |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
3405 |
As @MyDeveloperDay said before, eliding braces was (by far?) the most frequent review comments, and the community would appreciate something like this. So it's likely that this option will get used and experimented, and we will get bug reports and other feedback to improve the functionalities. Once we think it's ready, we can easily configure/package the functionalities for other styles. | |
3405 |
One idea is to run insertion for all missing braces and then this option to remove the optional ones for LLVM, i.e., an AutomaticBraces if you will. | |
clang/lib/Format/UnwrappedLineParser.cpp | ||
2433 | I see. I will think about it. | |
clang/unittests/Format/FormatTest.cpp | ||
23416 | Did you mean to add the expected part as a separate case? I don't think it would add any value if there are no braces to remove in the first place? |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2378 | MatchingParen is used to flag braces that may be optional. If it turns out that they should not be removed, we must reset it. Otherwise, the test case on line 23314 in FormatTest.cpp below would fail. |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
23416 |
Nvm. I think you wanted something like verifyFormat(PostformatCode, PreformatCode, Style)? Yes, I could do that, but I would have to relax the restriction to calling BracesRemover() in Format.cpp, i.e. checking isCpp() instead. |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2378 | Is MatchingParen for (if) braces null before this patch? Because I would expect that always to be set, if there is a matching brace and maybe base patches on that assumption. |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2378 | Yes, it's not set until it's in the token annotator. That's why I added the assertion in the annotator. |
- Fixed a bug found by applying the patch on the entire clang source tree.
- Added an exception: the braces are not removed if the right one is followed by a comment on the same line.
- Updated an existing test case and added a new one.
Fixing a couple of major bugs found by running check-clang plus minor bug fixes and cleanup:
- In parseLevel(), HasOpeningBrace and case tok::r_brace: don't necessarily mean the tokens are { and }, respectively.
- A pair of braces may be subject to removal even if the } is followed by a non-trailing comment.
- The newline before a wrapped { should be deleted when removing the braces.
- precededByCommentOrPPDirective() should be a const member function.
- The assertion that a { of a control statement block is not parsed more than once is false.
Now this patch not only successfully builds clang but also passes check-clang (by not failing more tests than the current clang-format).
Owen I think we should push ahead with this rather than D95168: [clang-format] Add Insert/Remove Braces option as I've looked at what you've done here I'm reminded that the removal and insertion are likely mutually exclusive operations.
There is no doubt that insertion is desired (so I'd like to see us do that afterwards) but I now think that could be in a completely separate pass as you originally suggested.
LGTM
Thanks! I will wait a couple of days in case other reviewers have some final requests.
- Use verifyFormat instead of EXPECT_EQ.
- Make this option have less impact on the unwrapped line parser by checking Style.RemoveBracesLLVM.
- Remove some unused code.
- Braces should not be removed if the single-statement block might wrap or is already wrapped.
- Added test cases.
- Did a final round of cleanup.
clang/include/clang/Format/Format.h | ||
---|---|---|
3059 | Can you elaborate? I simply copied the format of the existing warning block. The alignment of the \code block below is off by 1 though. | |
clang/lib/Format/UnwrappedLineParser.cpp | ||
466 | Most of the parse...() functions return void with no indication of pass/fail. Some of them return bool, e.g. parseObjCProtocol(), which doesn't indicate pass/fail according to the function header comment. I will add a similar comment. |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
3405 | Not yet. We probably want to test inserting braces Always on check-clang if it's not already been done. |
I don't think it's a good idea to make this option dependent on a particular style. Even if it works for LLVM style only for the moment.