MyDeveloperDay requested changes to this revision.
MyDeveloperDay added a comment.
This revision now requires changes to proceed.
Firstly thank you for the patch..
Prior authors have pointed out that new styles need to be supported by a large
public style guide as evidence as to why clang-format should support such
changes. (if you can find one that points to this being a preference I would
reference it here for completeness, see note below)
My personal view is we are getting to the point where so many large projects
are already clang-formated to some extent, the definition of the style guide
could be based on what clang-format can do and not on necessarily makes the
code more readable. What I have begun to notice is some projects on github
where clang-format can't support their style simply don't have a .clang-format
file. (and that could often be because of 1 or 2 issues which prevent them from
adopting it fully.) what is also strange is that that may have a .clang-tidy
file which points to them potentially being keen to adopt the clang tools, but
why not clang-format when its clearly the simplest to adaopt first)
The reason I ask is to uncover the motivation for adding the change? is it
something you need or a nice to have? (adding that to the revision descriptrion
helps the reviewers understand the motivation)
We'd normally ask that people be willing to support their revision should there
be any need to fix issues, I'd also say that clang-format desperately needs
additional developers to help fix issues and in particular to do code reviews,
I only say this because this is seemingly your first revision and whilst I
think this revision LGTM I'd hope we could call on you to help maintain
clang-format moving forward (given you have an ability to understand how it
works in order to do this patch)
Now regarding this revision. It wasn't initially clear what this change gave
us, I now I look deeper I see it as being the ability to break the brace when
the condition (if,for,catch,etc..) gets itself broken over multiple lines,
leading to the view that:
if (aaaaaaaaaaaaaaaaaaaaa
&& bbbbbbbbbbbbbbb
&& cccccccccccccc)
{
return true;
}
else {
return false;
}
is easier to read than:
if (aaaaaaaaaaaaaaaaaaaaa
&& bbbbbbbbbbbbbbb
&& cccccccccccccc) {
return true;
}
else {
return false;
}
whilst still recognizing the desire for the compact version when the condition
is small.
if (true) {
return true;
}
else {
return false;
}
To be honest, I'd be more than happy to see this style as it most closely
matches what I used to do all the time before I switched to clang-formatting on
save all the time (which meant I couldn't), That was to use a different bracing
style based on the complexity of the condition.
I guess my question is do you thin any of the other bracing wrapping rules need
the same level of control? (I can't think of any off the top of my head)
Apart from the missing documentation this LGTM (which is why I requested
changes), I'm not sure if others have a different view (maybe give them some
time to review this), but you'll find it can be quite quiet here. (which is why
we need more volunteers to do reviews).
If you can find that style guide evidence!
================
Comment at: clang/include/clang/Format/Format.h:817
+ BWACS_Always
+ };
+
----------------
You are missing a doc change in the clang/docs/ClangStyleOptions.rst file
(which I believe you can autogenerate from the Format.h), this needs to be part
of the patch
================
Comment at: clang/lib/Format/Format.cpp:643
+ true};
switch (Style.BreakBeforeBraces) {
case FormatStyle::BS_Linux:
----------------
Nit: this a drive by comment, but I feel this is really hard to read.. and not
something that needs to be done in this revision but this feels like it needs
to be formatted differently
{
/*AfterClass=*/false,
/*AfterEnum=*/false
/*AfterControlStatement=*/FormatStyle::BWACS_Never,
.....
}
because without going back and checking that the AfterControl statement is the
3rd bool I simply can't see what is on and what is off.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68296/new/
https://reviews.llvm.org/D68296
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
