[PATCH] D98429: [clang-format] Add new option to clang-format: SpaceBeforeForLoopSemiColon

[Nathan Hjelm via Phabricator via cfe-commits]

hjelmn added a comment.

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.



================
Comment at: clang/include/clang/Format/Format.h:2841
+  ///    true:                                  false:
+  ///    for (i = 0 ; i < 1 ; ++i) {}   vs.     for (i = 0; i < 1; ++i) {}
+  /// \endcode
----------------
HazardyKnusperkeks wrote:
> Please also show the range based for here. Otherwise it may be surprising, it would be for me.
Will do. Plan to get back to this this weekend.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:4004
     return false;
+  if (Right.is(TT_ForLoopSemiColon))
+    return false;
----------------
curdeius wrote:
> 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. 
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.


================
Comment at: clang/unittests/Format/FormatTest.cpp:12712
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a : b) {\n}", Space);
+}
----------------
curdeius wrote:
> HazardyKnusperkeks wrote:
> > hjelmn wrote:
> > > HazardyKnusperkeks wrote:
> > > > Okay that was unexpected for me, I thought the space would only apply to the old `for`s.
> > > > 
> > > > 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.
> > > 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?
> > 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.
> 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.
Makes sense to me. Will do that and will close this and other comment once that is complete.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98429/new/

https://reviews.llvm.org/D98429