This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add new option to clang-format: SpaceBeforeForLoopSemiColon
AbandonedPublic

Authored by HazardyKnusperkeks on Mar 11 2021, 8:23 AM.

Details

Summary

The option SpaceBeforeForLoopSemiColon controls whether clang-format puts
a space before the semi-colons in a for loop. When disabled (default) there
is no change to the current behavior. When enabled clang-format will put a
space before semi-colons in for loops:

enabled:

for (int i = 0 ; i < 10 ; ++i) {}
for (int i = 0 ; auto a : b) {}

disabled:

for (int i = 0; i < 10; ++i) {}
for (int i = 0; auto a : b) {}

Signed-off-by: Nathan Hjelm <hjelmn@google.com>

Diff Detail

Event Timeline

hjelmn requested review of this revision.Mar 11 2021, 8:23 AM
hjelmn created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 8:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hjelmn added a project: Restricted Project.Mar 14 2021, 7:55 PM

Please add a note to the release notes and also a test for a range based for with initializer (c++20).

hjelmn updated this revision to Diff 330800.Mar 15 2021, 1:59 PM
hjelmn edited the summary of this revision. (Show Details)

Updated change log and added a test to cover range based for with initializer.

hjelmn updated this revision to Diff 330834.Mar 15 2021, 4:08 PM

Fixed a typo

hjelmn updated this revision to Diff 330865.Mar 15 2021, 7:45 PM

One more issue with the test. Made a mistake with whether there is a space before the colon.

hjelmn edited the summary of this revision. (Show Details)Mar 15 2021, 7:47 PM
HazardyKnusperkeks requested changes to this revision.Mar 16 2021, 12:46 AM
HazardyKnusperkeks added inline comments.
clang/docs/ReleaseNotes.rst
168

No need for change, but in the future I would prefer to add changes to the end of the list. :)

clang/include/clang/Format/Format.h
2841

Please also show the range based for here. Otherwise it may be surprising, it would be for me.

clang/unittests/Format/FormatTest.cpp
12712

Okay that was unexpected for me, I thought the space would only apply to the old fors.

In that case please add while and maybe if with initializer. What should be discussed is if for and the other control statements with initializer should behave differently with regard to their initializer semicolon.

This revision now requires changes to proceed.Mar 16 2021, 12:46 AM
hjelmn added inline comments.Mar 16 2021, 8:28 AM
clang/docs/ReleaseNotes.rst
168

Opps. Should have asked the convention before putting it at the top. I will go ahead and move it to be consistent with LLVM practice.

clang/unittests/Format/FormatTest.cpp
12712

hmm, I think then I could rename this one to: SpaceBeforeCForLoopSemiColon which would then only add spaces in for(init;cond;increment)
then maybe SpaceAfterControlStatementInitializer or SpaceBeforeControlStatementSemiColon that controls the behavior for control statements with initializers.

How does that sound?

HazardyKnusperkeks added inline comments.
clang/docs/ReleaseNotes.rst
168

Don't know about LLVM practice, I'm just about 4 month here. Just my preference (and how I've done it until now).

clang/unittests/Format/FormatTest.cpp
12712

Apart from the names good. For the names I don't have anything really better right now.

But this is currently just my opinion, we should ask @MyDeveloperDay and @curdeius.

curdeius requested changes to this revision.Mar 16 2021, 2:10 PM

Could you point me to a style guide that formats the code in this way please?

clang/include/clang/Format/Format.h
2838

A better description would be nice, here only the false case is described. I'd rather see something that describes the option is in general and adds that true will add/keep a space.

clang/lib/Format/TokenAnnotator.cpp
4004

Is this change really necessary? Is there a test for it?
I guess that a semicolon should return false because otherwise it could be put after the line break, but you certainly need a test for that.

clang/unittests/Format/FormatTest.cpp
12712

I would prefer to avoid multiplying the different options and regroup all of the control statements under a single one.
SpaceBeforeControlStatementSemicolon sounds acceptable with one nit, I'd use "Semicolon" (as a single word, with lowercase 'c') to match the usage in LLVM (e.g. in clang-tidy).

WDYT about a single option for all statements?
Another way is to add a separate option for each statement type. Or, use a (bitmask like) enum with a single option. The latter can be done in a backward compatible way in the future BTW.

This is something I have been doing for over 20 years now. Not sure when I initially picked it up but I find a space before the ;'s in a C for loop improves readability. It more clearly differentiates the different parts. I beleive the space before the colon in C++ range-based loops is based on the same readability improvement.

Anyway, I intend to use clang-format in a couple of C projects and want to make sure it doesn't try to remove the intentional formatting of the code. I have at least one more CL I need to open up for a bug I found in the formatting. Will try to get that one open as soon as I get this one finished.

clang/include/clang/Format/Format.h
2841

Will do. Plan to get back to this this weekend.

clang/lib/Format/TokenAnnotator.cpp
4004

Yes. The way I implemented it this I think it was necessary. I will have to double-check. I plan to re-do this with an enum so will try to avoid extraneous changes.

clang/unittests/Format/FormatTest.cpp
12712

Makes sense to me. Will do that and will close this and other comment once that is complete.

hjelmn marked an inline comment as not done.Mar 19 2021, 6:48 PM

Can you show for (;;) {} in your tests?

HazardyKnusperkeks commandeered this revision.Nov 3 2022, 5:51 AM
HazardyKnusperkeks edited reviewers, added: hjelmn; removed: HazardyKnusperkeks.
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 5:51 AM
HazardyKnusperkeks abandoned this revision.Nov 3 2022, 5:52 AM

More than a year silence.