diff mbox series

[5/8] clang-format: avoid braces on simple single-statement bodies

Message ID 20240708092317.267915-6-karthik.188@gmail.com (mailing list archive)
State Superseded
Headers show
Series clang-format: add more rules and enable on CI | expand

Commit Message

karthik nayak July 8, 2024, 9:23 a.m. UTC
Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
braces for single-statement bodies in conditional blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Junio C Hamano July 8, 2024, 4:48 p.m. UTC | #1
Karthik Nayak <karthik.188@gmail.com> 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).

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.


[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."

> +RemoveBracesLLVM: true
karthik nayak July 8, 2024, 8:25 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> 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.
Phillip Wood July 12, 2024, 3:24 p.m. UTC | #3
Hi Karthik

On 08/07/2024 21:25, Karthik Nayak wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Karthik Nayak <karthik.188@gmail.com> 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.
karthik nayak July 13, 2024, 12:30 p.m. UTC | #4
Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Karthik
>
> On 08/07/2024 21:25, Karthik Nayak wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> 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?
>

Well, it mostly means that the CI would show the style-check job as
failed, but the overall pipeline would be successful. We want to ideally
fail the pipeline too, but I'm being careful to not disrupt things
suddenly and I think once we see what false positive rate is and once we
fine tune this settings maybe over the next release or so, we can
enforce this.

[snip]
Phillip Wood July 13, 2024, 1:58 p.m. UTC | #5
On 13/07/2024 13:30, Karthik Nayak wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
 >
>> 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?
>>
> 
> Well, it mostly means that the CI would show the style-check job as
> failed, but the overall pipeline would be successful.

Ah, I didn't know that was a possibility - do users still get emails 
about the failed job?

> We want to ideally
> fail the pipeline too, but I'm being careful to not disrupt things
> suddenly and I think once we see what false positive rate is and once we
> fine tune this settings maybe over the next release or so, we can
> enforce this.

Starting gently to get some experience with the auto formatter sounds 
sensible. Thanks for working on this, I really hope that once we've got 
more experience with clang format we can figure out how to enable it 
unconditionally.

Best Wishes

Phillip
karthik nayak July 13, 2024, 2:16 p.m. UTC | #6
phillip.wood123@gmail.com writes:

> On 13/07/2024 13:30, Karthik Nayak wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>  >
>>> 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?
>>>
>>
>> Well, it mostly means that the CI would show the style-check job as
>> failed, but the overall pipeline would be successful.
>
> Ah, I didn't know that was a possibility - do users still get emails
> about the failed job?
>

I know in GitLab, users _wouldn't_ get an email. This would be a nice
intermediate step, but I think it is still okay.

>> We want to ideally
>> fail the pipeline too, but I'm being careful to not disrupt things
>> suddenly and I think once we see what false positive rate is and once we
>> fine tune this settings maybe over the next release or so, we can
>> enforce this.
>
> Starting gently to get some experience with the auto formatter sounds
> sensible. Thanks for working on this, I really hope that once we've got
> more experience with clang format we can figure out how to enable it
> unconditionally.
>
> Best Wishes
>
> Phillip

Just to clarify, enabling an auto-formatter would always be left to the
user. From the project's perspective, the goal would be to have checks
to notify us (us as in the Git project) if a particular patch series
fails to comply to our standards.

This is done by enforcing the CI rule _eventually_. Users can also
benefit from this if they use the CI for development purposes. One good
example being users of GitGitGadget, where they would be notified of
failures in the CI. Another example is contributions coming from GitLab,
where our team uses the CI before sending patches upstream.

Since the Git project already comes with a '.clang-format', users can
already enable auto-formatting on their end. I know the clangd LSP
already supports this. ;)

Thanks
Karthik
diff mbox series

Patch

diff --git a/.clang-format b/.clang-format
index 914254a29b..1a5f0c9046 100644
--- a/.clang-format
+++ b/.clang-format
@@ -117,6 +117,11 @@  IndentWrappedFunctionNames: false
 # int *a;
 PointerAlignment: Right
 
+# 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.
+RemoveBracesLLVM: true
+
 # Don't insert a space after a cast
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false