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