| Message ID | 20240711083043.1732288-6-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-format: add more rules and enable on CI | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > 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. > > The latter seems to be of concern. But since we only use clang-format to > verify the format and not to apply formatting, we should be okay here. Hmph. Could you tell me where I can read more about "we tell clang-format only to verify but not to apply"? If that is truely the case, perhaps I shouldn't be worried to much, but it is not clear to me how we enforce that this is to be used only for verification with non-zero false positive, and never for reformatting before submission. The senario I was worried about was this. We aadd to .clang-format that is in-tree, and not just CI jobs but our human contributors may use it to check what they newly wrote before committing and they may even take the differences as suggested fixes (which may end up breaking their working code). Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> 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. >> >> The latter seems to be of concern. But since we only use clang-format to >> verify the format and not to apply formatting, we should be okay here. > > Hmph. Could you tell me where I can read more about "we tell > clang-format only to verify but not to apply"? If that is truely > the case, perhaps I shouldn't be worried to much, but it is not > clear to me how we enforce that this is to be used only for > verification with non-zero false positive, and never for > reformatting before submission. > I was referring to the fact that, we expose '.clang-format' via 'make style' which only prints the diff to the STDOUT. The user has to still manually make these changes. However users could be using tools to auto-format on save and this could be an issue. > The senario I was worried about was this. We aadd to .clang-format > that is in-tree, and not just CI jobs but our human contributors may > use it to check what they newly wrote before committing and they may > even take the differences as suggested fixes (which may end up > breaking their working code). > > Thanks. I totally see your point here. If the contributors do end up with bad formatting because clang messed it up, that is an okay situation, since that shouldn't happen often, and when it does, it would be the same situation as without this check, wherein we rely on reviewers. The issue is whether it would break their code, I couldn't find anything on this. But overall, while I personally find this check useful, I'm happy to drop it, My goal is to ensure we run this on CI as a first step :)
Karthik Nayak <karthik.188@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Karthik Nayak <karthik.188@gmail.com> writes: >> >>> 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. >>> >>> The latter seems to be of concern. But since we only use clang-format to >>> verify the format and not to apply formatting, we should be okay here. >> >> Hmph. Could you tell me where I can read more about "we tell >> clang-format only to verify but not to apply"? If that is truely >> the case, perhaps I shouldn't be worried to much, but it is not >> clear to me how we enforce that this is to be used only for >> verification with non-zero false positive, and never for >> reformatting before submission. >> > > I was referring to the fact that, we expose '.clang-format' via 'make > style' which only prints the diff to the STDOUT. The user has to still > manually make these changes. > > However users could be using tools to auto-format on save and this could > be an issue. Yes. Of course if we really wanted to avoid end-user confusion and still want to have this in CI (if only to see how well the rule fares, and what the actual false-positive rate is), we _could_ run CI's job with custom .clang-format file that is not visible to end-users in their normal checkout, or something silly like that. If we are going to use it, then we should use it everywhere, making sure everybody is careful. If the cost of forcing everybody to be careful is too high, we may want to retract it, but we won't know until we try. Thanks.
On 12/07/2024 16:09, Junio C Hamano wrote: > Of course if we really wanted to avoid end-user confusion and still > want to have this in CI (if only to see how well the rule fares, and > what the actual false-positive rate is), we _could_ run CI's job > with custom .clang-format file that is not visible to end-users in > their normal checkout, or something silly like that. Getting some idea of how useful the auto-formatter is would be valuable I think. > If we are > going to use it, then we should use it everywhere, making sure > everybody is careful. If the cost of forcing everybody to be > careful is too high, we may want to retract it, but we won't know > until we try. It would be great if we could find a way to auto-format the code without inconveniencing contributors. Best Wishes Phillip
diff --git a/.clang-format b/.clang-format index 914254a29b..8b75dece80 100644 --- a/.clang-format +++ b/.clang-format @@ -117,6 +117,12 @@ 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 but keeps braces if one side of +# if/else if/.../else cascade has multi-statement body. +RemoveBracesLLVM: true + # Don't insert a space after a cast # x = (int32)y; not x = (int32) y; SpaceAfterCStyleCast: false
Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly braces for single-statement bodies in conditional blocks. This option does come with two warnings: This option will be renamed and expanded to support other styles. and 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. The latter seems to be of concern. But since we only use clang-format to verify the format and not to apply formatting, we should be okay here. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- .clang-format | 6 ++++++ 1 file changed, 6 insertions(+)