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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
