Hi Karthik
On 08/07/2024 21:25, Karthik Nayak wrote:
Junio C Hamano <gitster@xxxxxxxxx> writes:
Karthik Nayak <karthik.188@xxxxxxxxx> writes:
Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
braces for single-statement bodies in conditional blocks.
Hmph, two warnings in its documentation [*] sound ominous, especially
the second one that says:
Warning
Setting this option to true could lead to incorrect code
formatting due to clang-format’s lack of complete semantic
information. As such, extra care should be taken to review code
changes made by this option.
which implies it may not necessarily a good idea to add to
automation without telling contributors that they may get hit with a
false positive (or incorrect rewrite).
Agreed on this one. I'm a bit skeptical to be honest too. I think I
should have added information about the warning in the commit too. I
will for next round. Overall, this also contributes to the reason why I
decided these CI jobs need to be allowed to fail.
I'm a bit confused - what does "allowed to fail" mean? We don't have any
automated requirements on the CI passing. We don't want to train
contributors to routinely ignore CI failures but if they don't get a
notification that this job failed how do they know to correct the
formatting?
Reading from the examples in that documentation page, it was unclear
how it would handle if/else if/.../else cascade where not all branches
are multi-statement blocks, e.g.,
if (A) {
do_A_thing();
} else if (B) {
do_B_thing();
} else {
do_C_things();
do_other_things();
}
but looking around I am getting a feeling that the tool would do the
right thing, i.e., to match our preference that is to use {braces}
around all branches, if I am not mistaken.
Yup, that was my understanding and what I could see from some quick
trials that I did too.
It would be a great win to have this though, because it is one of the
things that always get me.
Yes, having the formatting automated would be really helpful. It is a
shame the documentation doesn't really explain what the issue is (i.e.
when does it generate invalid code) so we don't know how likely we are
to be affected by it.
Best Wishes
Phillip
[Reference]
* https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#:~:text=RemoveBracesLLVM
+# Remove optional braces of control statements (if, else, for, and while)
+# according to the LLVM coding style
+# This avoids braces on simple single-statement bodies of statements.
"... but keeps braces if one side of if/else if/.../else cascade has
multi-statement body."
Makes sense, will add.
+RemoveBracesLLVM: true
Overall I must say, I'd be okay if this patch is also dropped from this
series.
