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.
>
> 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.
>
> [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.
Attachment:
signature.asc
Description: PGP signature
