| Message ID | 20240708092317.267915-2-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-format: add more rules and enable on CI | expand |
On Mon, Jul 8, 2024 at 2:23 AM Karthik Nayak <karthik.188@gmail.com> wrote: > Let's pick the former and add 'IndentPPDirectives: AfterHash' value to > our '.clang-format'. There is no clear reason to pick one over the > other, but it would definitely be nicer to be consistent. There is one extremely weak technical reason to pick "# in left column", which is: some ancient C compilers required it. I don't think any modern ones do. Chris
Karthik Nayak <karthik.188@gmail.com> writes: > We do not have a rule around the indentation of preprocessor directives. > This was also discussed on the list [1], noting how there is often > inconsistency in the styling. While there was discussion, there was no > conclusion around what is the preferred style here. One style being > indenting after the hash: > > #if FOO > # if BAR > # include <foo> > # endif > #endif > > The other being before the hash: > > #if FOO > #if BAR > #include <foo> > #endif > #endif > > Let's pick the former and add 'IndentPPDirectives: AfterHash' value to > our '.clang-format'. There is no clear reason to pick one over the > other, but it would definitely be nicer to be consistent. When I experimented with reindenting the CPP directives in <git-compat-util.h> [*], I think I saw an existing violation in an early part of the file. Outside the borrowed code in compat/, we have these: $ git grep -e '^[ ]\{1,\}#' master -- ':!compat/' \*.[ch] blame.c: #define FINGERPRINT_FILE_THRESHOLD 10 block-sha1/sha1.c: #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val)) block-sha1/sha1.c: #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while (0) block-sha1/sha1.c: #define setW(x, val) (W(x) = (val)) diff.h: #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA diff.h: #define COLOR_MOVED_MIN_ALNUM_COUNT 20 diff.h: #define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5) diff.h: #define COLOR_MOVED_WS_ERROR (1<<0) git-compat-util.h: #define GIT_GNUC_PREREQ(maj, min) 0 pkt-line.c: #define hex(a) (hexchar[(a) & 15]) pkt-line.c: #undef hex sha1dc/sha1.c: #define sha1_load(m, t, temp) { temp = m[t]; } sha1dc/sha1.c: #define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); } Should we clean them up before we start adding these rules to the file, especially if we plan to run the rules for stylistic check? Otherwise wouldn't we see noises coming from the existing lines that may dwarf the new ones, whose addition we want prevent? If we were to run the check in CI to help contributors, I would assume that you are arranging it to only complain about the lines touched by the commits they are contributing, not about the existing style violations. This comment is not limited to the CPP directive indentation but any other style rules .clang-format defines. If we are not checking only for lines affected by commits being contributed, then perhaps we should exclude compat/ from these rules? Thanks for working on this. [Reference] * https://lore.kernel.org/git/xmqqjzim197j.fsf@gitster.g/
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> We do not have a rule around the indentation of preprocessor directives. >> This was also discussed on the list [1], noting how there is often >> inconsistency in the styling. While there was discussion, there was no >> conclusion around what is the preferred style here. One style being >> indenting after the hash: >> >> #if FOO >> # if BAR >> # include <foo> >> # endif >> #endif >> >> The other being before the hash: >> >> #if FOO >> #if BAR >> #include <foo> >> #endif >> #endif >> >> Let's pick the former and add 'IndentPPDirectives: AfterHash' value to >> our '.clang-format'. There is no clear reason to pick one over the >> other, but it would definitely be nicer to be consistent. > > When I experimented with reindenting the CPP directives in > <git-compat-util.h> [*], I think I saw an existing violation in an > early part of the file. Outside the borrowed code in compat/, we > have these: > > $ git grep -e '^[ ]\{1,\}#' master -- ':!compat/' \*.[ch] > blame.c: #define FINGERPRINT_FILE_THRESHOLD 10 > block-sha1/sha1.c: #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val)) > block-sha1/sha1.c: #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while (0) > block-sha1/sha1.c: #define setW(x, val) (W(x) = (val)) > diff.h: #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA > diff.h: #define COLOR_MOVED_MIN_ALNUM_COUNT 20 > diff.h: #define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5) > diff.h: #define COLOR_MOVED_WS_ERROR (1<<0) > git-compat-util.h: #define GIT_GNUC_PREREQ(maj, min) 0 > pkt-line.c: #define hex(a) (hexchar[(a) & 15]) > pkt-line.c: #undef hex > sha1dc/sha1.c: #define sha1_load(m, t, temp) { temp = m[t]; } > sha1dc/sha1.c: #define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); } > > Should we clean them up before we start adding these rules to the > file, especially if we plan to run the rules for stylistic check? > Otherwise wouldn't we see noises coming from the existing lines that > may dwarf the new ones, whose addition we want prevent? > Making syntax changes just so the rule works was something I did consider, but I avoided it mostly because the CI only applies to the changes made and not pre-existing files. This also allows us to apply the boy scout rule and cleanup as we go. > If we were to run the check in CI to help contributors, I would > assume that you are arranging it to only complain about the lines > touched by the commits they are contributing, not about the existing > style violations. This comment is not limited to the CPP directive > indentation but any other style rules .clang-format defines. > Yes exactly, the usage of 'git-clang-format' allows us to only check for lines changed. > If we are not checking only for lines affected by commits being > contributed, then perhaps we should exclude compat/ from these > rules? > > Thanks for working on this. > > [Reference] > > * https://lore.kernel.org/git/xmqqjzim197j.fsf@gitster.g/
diff --git a/.clang-format b/.clang-format index 3ed4fac753..5e128519bf 100644 --- a/.clang-format +++ b/.clang-format @@ -96,6 +96,12 @@ BreakStringLiterals: false # Switch statement body is always indented one level more than case labels. IndentCaseLabels: false +# Indents directives before the hash. +# #if FOO +# # include <foo> +# #endif +IndentPPDirectives: AfterHash + # Don't indent a function definition or declaration if it is wrapped after the # type IndentWrappedFunctionNames: false
We do not have a rule around the indentation of preprocessor directives. This was also discussed on the list [1], noting how there is often inconsistency in the styling. While there was discussion, there was no conclusion around what is the preferred style here. One style being indenting after the hash: #if FOO # if BAR # include <foo> # endif #endif The other being before the hash: #if FOO #if BAR #include <foo> #endif #endif Let's pick the former and add 'IndentPPDirectives: AfterHash' value to our '.clang-format'. There is no clear reason to pick one over the other, but it would definitely be nicer to be consistent. [1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- .clang-format | 6 ++++++ 1 file changed, 6 insertions(+)