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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.

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



================
Comment at: clang/include/clang/Format/Format.h:2838
 
+  /// If ``false``, spaces will be removed before semi-colons in for loops.
+  /// \code
----------------
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.


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


================
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);
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98429