Junio C Hamano <gitster@xxxxxxxxx> writes: > Karthik Nayak <karthik.188@xxxxxxxxx> writes: > >> For 'clang-format', setting 'RemoveBracesLLVM' to 'true', adds a check >> to ensure we avoid curly braces for single-statement bodies in >> conditional blocks. >> >> However, the option does come with two warnings [1]: >> >> 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. While we want to experiment with the >> rule, adding it to the in-tree '.clang-format' could affect end-users. >> Let's only add it to the CI jobs for now. With time, we can evaluate >> its efficacy and decide if we want to add it to '.clang-format' or >> retract it entirely. We do so, by adding the existing rules in >> '.clang-format' and this rule to a temp file outside the working tree, >> which is then used by 'git clang-format'. This ensures we don't murk >> with files in-tree. >> >> [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm >> >> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx> >> --- >> ci/run-style-check.sh | 19 ++++++++++++++++++- >> 1 file changed, 18 insertions(+), 1 deletion(-) > > Have we done enough experiment by now and don't we want to move it > to the set of rules in our tree? > Christian and I were talking about clang-format today. So great to see that you too were thinking about it. I think this particular rule should be okay to move our tree. For this particular rule, I found one spot it fails: $ ./ci/run-style-check.sh @~1 diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 1809539201..341d9eac0d 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1929,12 +1929,11 @@ static void execute_commands_non_atomic(struct command *commands, goto cleanup; failure: - for (cmd = commands; cmd; cmd = cmd->next) { + for (cmd = commands; cmd; cmd = cmd->next) if (reported_error) cmd->error_string = reported_error; else if (strmap_contains(&failed_refs, cmd->ref_name)) cmd->error_string = strmap_get(&failed_refs, cmd->ref_name); - } cleanup: ref_transaction_free(transaction); We generally add brackets in such scenarios, but the style-check suggests to use them instead. > How often do people use "git clang-format" before they submit their > series these days, by the way? > To be honest, I don't think anyone does. As far as I've seen there is too many false positives currently. >> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh >> index 76dd37d22b..6cd4b1d934 100755 >> --- a/ci/run-style-check.sh >> +++ b/ci/run-style-check.sh >> @@ -5,4 +5,21 @@ >> >> baseCommit=$1 >> >> -git clang-format --style file --diff --extensions c,h "$baseCommit" >> +# 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. >> +# >> +# As this rule comes with a warning [1], we want to experiment with it >> +# before adding it in-tree. since the CI job for the style check is allowed >> +# to fail, appending the rule here allows us to validate its efficacy. >> +# While also ensuring that end-users are not affected directly. >> +# >> +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm >> +{ >> + cat .clang-format >> + echo "RemoveBracesLLVM: true" >> +} >/tmp/clang-format-rules >> + >> +git clang-format --style=file:/tmp/clang-format-rules \ >> + --diff --extensions c,h "$baseCommit"
Attachment:
signature.asc
Description: PGP signature
