patch, 3 partsDocumentation: some coding guideline updates

Hi,

I've had it in my mind for a long time to update our coding guideline for some things that I frequently stumble over during code reviews. This small patch series fills in those gaps.

Thanks!

Patrick

Patrick Steinhardt (3):
  Documentation: clarify indentation style for C preprocessor directives
  Documentation: document naming schema for struct-related functions
  Documentation: document difference between release and free

 Documentation/CodingGuidelines | 41 ++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

There has recently been some discussion around how C preprocessor directives shall be indented [1]. This discussion was settled towards indenting after the hash by two spaces [2]. Document it as such.

[1]: https://lore.kernel.org/all/xmqqwmmm1bw6.fsf@gitster.g/ [2]: https://lore.kernel.org/all/20240708092317.267915-2-karthik.188@gmail.com/

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/CodingGuidelines | 10 ++++++++++
 1 file changed, 10 insertions(+)

Patrick Steinhardt <ps@pks.im> writes:

It was settled to have space after and not before the hash, but I do not recall ever agreeing to two spaces. I prefer to increment by 1 for each level instead to keep the whole thing less distracting while carrying meaningful information.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Using the indentation consistently is a good thing I do not object to.

Among our existing header and source files (excluding compat/ that is full of borrowed sources), scanning output from

    $ git grep -e '#  *' '*.[ch]' ':!compat/'

tells me that

 - builtin/gc.c (3 lines)
 - git-compat-util.h (52 lines)
 - trace.c (2 lines)
 - wildmatch.c (6 lines)

use one space indent after '#' per level, while

 - hash.h (10 lines, inconsistently 2 uses 1 per level)
 - thread-utils.c (8 lines)

use two space indent after '#' per level.

In compat/ directory, only compat/nedmalloc uses 2-space indent. There in the hierarchy are so many files we borrowed from GNU, whose coding style sticks to one-space indent.

On Wed, Jul 24, 2024 at 10:06:03PM -0700, Junio C Hamano wrote:

Thanks for checking. I don't really mind whether we use one or two spaces, as long as we are being consistent. I actually took the 2-space indent from the comment for "IndentPPDirectives" in ".clang-format", where it looks as if clang-format was using two spaces. So I took this as being the official style we settled on.

One thing I noticed though is that clang-format is inconsistent with either of those styles because it uses a tab character to indent nested preprocessor directives. So the comment we have in that file is quite misleading. We can fix this with the below change though. I'll add that to this series.

Patrick

We nowadays have a proper mishmash of struct-related functions that are called `<verb>_<struct>` (e.g. `clear_prio_queue()`) versus functions that are called `<struct>_<verb>` (e.g. `strbuf_clear()`). While the former style may be easier to tie into a spoken conversation, most of our communication happens in text anyway. Furthermore, prefixing functions with the name of the structure they operate on makes it way easier to group them together, see which functions are related, and will also help folks who are using code completion.

Let's thus settle on one style, namely the one where functions start with the name of the structure they operate on.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/CodingGuidelines | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Patrick Steinhardt <ps@pks.im> writes:

Nit: It would be nice to add `<struct>_<verb>` here, since we do something
similar in the next patch.

I first thought this was a typo and should've been `add_strbuf` instead, but I'm sure your intention was to show _other forms_ instead of `<struct>_<verb>` here.

Maybe we could do s/is preferred over/is preferred over other forms, like/. I dunno. It is probably fine as is :)

On Wed, Jul 24, 2024 at 04:42:29AM -0700, Karthik Nayak wrote:

Makes sense, will do.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

I am not sure if this is a good guideline. In the case of strbuf_, you could say it is named after the structure, but I would actually think that both structure and the functions are named after the subsystem/API (i.e. we have "strbuf" that other subsystems can use).

Do we want to rename start_command(), finish_command(), run_command() and pipe_command()? child_process_start() sounds somewhat ungrammatical.

By the way, some functions that have strbuf_ in their names do not have anything to do with managing strings using the strbuf structure, but they do things that are *not* about strings, but happen to use strbuf as a way to either feed input to them or carry output out of them. They should be renamed away to lose "strbuf_" in their names (e.g. strbuf_realpath() is about pathnames; it is immaterial that the function happens to use strbuf to hold its output but takes input from "const char *").

Junio C Hamano <gitster@pobox.com> writes:

Another thing we may want to say about these "The primary data structure subsystem 'S' deals with is called 'struct S' and the functions that operate on 'struct S' are named S_<verb>()" theme is that the convention for S_<verb>() functions is to have the operand, which is always 'struct S *', of the verb as the first parameter.

On Wed, Jul 24, 2024 at 09:50:40AM -0700, Junio C Hamano wrote:

Well, in most cases I'd expect that the structure is named after the subsystem/API, itself. I'm happy to relax this statement though and say that functions should be named after the subsystem.

I wouldn't quite go that far for now. We may want to slowly adapt some parts of our interfaces over time. But my main goal is rather to make the style consistent for _new_ interfaces we add.

It does, but I would argue that it is no different from `strbuf_reset()` and other functions where we have the verb as a trailer. And I have to say that I find it a ton easier to reason about code where we have the subsystem it belongs to as a prefix as it neatly groups together things and immediately sets you into the correct mindset of what to expect. That is of course a question of preference, I'm not claiming that my preferral is objectively the best.

But again, what I do want to see is consistency. Nobody is helped when we mix both styles in my opinion. It makes writing, reading and reviewing code harder than it has to be because you always have to remember whether it is `string_list_free()`, `free_string_list()`, `string_list_clear()` or `clear_string_list()`.

Yeah, that's fair indeed.

Patrick

We semi-regularly have discussions around whether a function shall be named `release()` or `free()`. For most of the part we use these two terminologies quite consistently though:

  - `release()` only frees internal state of a structure, whereas the
    structure itself is not free'd.

  - `free()` frees both internal state and the structure itself.

Carve out a space where we can add idiomatic names for common functions in our coding guidelines. This space can get extended in the future when we feel the need to document more idiomatic names.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/CodingGuidelines | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Patrick Steinhardt <ps@pks.im> writes:

I noticed there is also `clear()` used in some places. Should we also mention that we don't recommend using `clear()` WRT freeing memory?

The patch itself looks good.

On Wed, Jul 24, 2024 at 04:46:20AM -0700, Karthik Nayak wrote:

In any case I think we should decide on eithe using `clear()` or using `release()` for consistency's sake. Which of both we use I don't quite care, but the following very shoddy analysis clearly favors `release()`:

    $ git grep '_clear(' | wc -l
    844
    $ git grep '_release(' | wc -l
    2126

So yeah, I'm happy to explicitly mention that `clear()` shouldn't be used in favor of `release()`.

Patrick

Hi Patrick

On 24/07/2024 14:11, Patrick Steinhardt wrote:

I think a fairer comparison would be to look at function declarations, not all the call sites.

$ { git grep 'void [a-z_]*_release(' '*.h'
     git grep 'static void [a-z_]*_release(' '*.c'
   } | wc -l
47
$ { git grep 'void [a-z_]*_clear(' '*.h'
     git grep 'static void [a-z_]*_clear(' '*.c'
   } | wc -l
58

So we have more _clear() functions than _release() functions. I think there may sometimes be a semantic difference between _clear() and _release() as well where some _clear() functions zero out the struct after freeing the members.

Thanks for working on this it will be a useful addition to our coding guidelines

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Thanks for doing a more thorough study of the current codebase. I tend to agree that the number of actual _clear() functions matter a lot more than how many callsites call _clear(), and it would make sense to standardise on it. If everything else being equal, it does not matter which one we pick, but it rarely happens that everything else is equal.

 - "release" is a bit more cumbersome to type and read than "clear".

 - "clear" at least to me says more about the state of the thing
   after it got cleared (e.g., I would expect it would be filled
   with NUL bytes)

 - "release" places a lot more stress on what happens to the things
   that were contained before the release takes place.

For example, upon either "clear" or "release", I would expect everything pointed by elements in an array member of the struct, and the array pointed at by the member, are free'd when we are "clearing/releasing" a strvec. But I may not care what is left in it after "release". It can be left to hold all the bytes the struct had before "release" got called, as anybody who called the function are not supposed to look at the struct again anyway. But we may choose not to have such a variant and always clear the struct after releasing resources it held, just for good hygiene.

So in short, I would consider that "clear = release + init". If we want to have both "clear" and "release" and have them distinct meaning, that is fine. If we want to simplify and do without "just release and leave them dirty" variant, then we need only one name for it, and I do not mind if we called it "release", even though I would think "clear" is a better name for the action that behaves as if "init" was done at the end to make it reusable.

Thanks.

On Wed, Jul 24, 2024 at 11:02:34AM -0700, Junio C Hamano wrote:

I'm not quite sure that I agree with this. I think coding style is most heavily influenced by what you see most in a codebase. So I'd argue that it is both declarations/definitions and callsites that influence the general shape.

This of course means that interfaces like `struct strbuf` have way more impact on our coding style than others, simply because it is being used all over the place. But in my opinion that follows naturally, because the coding style that we use should work best for what is being used most often.

But anyway, this is splitting hairs :)

I actually like this definition. The only downside I see of defining `clear = release + init` is that `init()` probably shouldn't be allowed to allocate any memory in this case. Otherwise, calling `clear()` on a structure would not cause us to free all resources associated with it, which would be unexpected to me.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

We have _clear() in some subsystem/API. Are we sure the listed two are sufficient and _clear() can be replaced with one of them (perhaps _release())?

On Wed, Jul 24, 2024 at 09:52:20AM -0700, Junio C Hamano wrote:

I'd think that `clear()` can be replaced by `release()`, yes. But in another branch I heard the argument that `clear()` is equivalent to `release()` followed by `init()`, which I do like. The only downside is that `init()` must not allocate memory in this case, as otherwise the `clear()` function would lose the ability to release all resources associated with its structure.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

Thanks for the initiative. I've left some small comments, but overall this already looks good to me.

Karthik

Hi,

this is the second version of my patch series that aims to improve our coding guidelines such that we arrive at a more consistent coding style.

Changes compared to v1:

  - Fix clang-format to use a single space to indent preprocessor
    directives instead of using tabs. Thus, this series is now built
    with kn/ci-clang-format at 1b8f306612 (ci/style-check: add
    `RemoveBracesLLVM` in CI job, 2024-07-23) merged into v2.46.0.

  - Adapt the coding guidelines accordingly to also only use a single
    space for indentation of nested preprocessor directives.

  - Adopt a proposal by Junio to more clearly spell out the relationship
    between a subsystem `S`, `struct S` and its functions `S_<verb>()`.

  - Document `S_clear()`-style functions. I have adopted the proposal by
    Junio hear, where `clear = release + init` with the restriction that
    `S_init()` must not allocate any resources.

  - Add another patch on top that makes variable initializers consistent
    in our coding guidelines. Our style is to add spaces between the
    curly brace and the initializers (`struct foo bar = { something };`).

I think I captured everything that came out of the discussion, but please let me know in case I misinterpreted or forgot anything.

Thanks!

Patrick

Patrick Steinhardt (5):
  clang-format: fix indentation width for preprocessor directives
  Documentation: clarify indentation style for C preprocessor directives
  Documentation: document naming schema for structs and their functions
  Documentation: document idiomatic function names
  Documentation: consistently use spaces inside initializers

 .clang-format                  |  6 +++--
 Documentation/CodingGuidelines | 48 +++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 3 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  c33ad700d6 clang-format: fix indentation width for preprocessor directives
1:  64e0b44993 ! 2:  e3baf01234 Documentation: clarify indentation style for C preprocessor directives
    @@ Metadata
      ## Commit message ##
         Documentation: clarify indentation style for C preprocessor directives
     
    -    There has recently been some discussion around how C preprocessor
    -    directives shall be indented [1]. This discussion was settled towards
    -    indenting after the hash by two spaces [2]. Document it as such.
    -
    -    [1]: https://lore.kernel.org/all/xmqqwmmm1bw6.fsf@gitster.g/
    -    [2]: https://lore.kernel.org/all/20240708092317.267915-2-karthik.188@gmail.com/
    +    In the preceding commit, we have settled on using a single space per
    +    nesting level to indent preprocessor directives. Clarify our coding
    +    guidelines accordingly.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ Documentation/CodingGuidelines: For C programs:
       - We use tabs to indent, and interpret tabs as taking up to
         8 spaces.
      
    -+ - Nested C preprocessor directives are indented after the hash by two
    -+   spaces per nesting level.
    ++ - Nested C preprocessor directives are indented after the hash by one
    ++   space per nesting level.
     +
     +	#if FOO
    -+	#  include <foo.h>
    -+	#  if BAR
    -+	#    include <bar.h>
    -+	#  endif
    ++	# include <foo.h>
    ++	# if BAR
    ++	#  include <bar.h>
    ++	# endif
     +	#endif
     +
       - We try to keep to at most 80 characters per line.
2:  7f07bf1f3b ! 3:  25f73b970d Documentation: document naming schema for struct-related functions
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    Documentation: document naming schema for struct-related functions
    +    Documentation: document naming schema for structs and their functions
     
         We nowadays have a proper mishmash of struct-related functions that are
         called `<verb>_<struct>` (e.g. `clear_prio_queue()`) versus functions
    @@ Documentation/CodingGuidelines: For C programs:
         use your own debugger and arguments. Example: `GIT_DEBUGGER="ddd --gdb"
         ./bin-wrappers/git log` (See `wrap-for-bin.sh`.)
      
    -+ - Functions that operate on a specific structure and which are used by
    -+   other subsystems shall be named after the structure. The function
    -+   name should start with the name of the structure followed by a verb.
    -+   E.g.
    ++ - The primary data structure that a subsystem 'S' deals with is called
    ++   `struct S`. Functions that operate on `struct S` are named
    ++   `S_<verb>()` and should generally receive a pointer to `struct S` as
    ++   first parameter. E.g.
     +
     +	struct strbuf;
     +
3:  5e1de3c315 ! 4:  d4ce00303f Documentation: document difference between release and free
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    Documentation: document difference between release and free
    +    Documentation: document idiomatic function names
     
         We semi-regularly have discussions around whether a function shall be
    -    named `release()` or `free()`. For most of the part we use these two
    -    terminologies quite consistently though:
    -
    -      - `release()` only frees internal state of a structure, whereas the
    -        structure itself is not free'd.
    -
    -      - `free()` frees both internal state and the structure itself.
    +    named `S_release()`, `S_clear()` or `S_free()`. Indeed, it may not be
    +    obvious which of these is preferable as we never really defined what
    +    each of these variants means exactly.
     
         Carve out a space where we can add idiomatic names for common functions
    -    in our coding guidelines. This space can get extended in the future when
    -    we feel the need to document more idiomatic names.
    +    in our coding guidelines and define each of those functions. Like this,
    +    we can get to a shared understanding of their respective semantics and
    +    can easily point towards our style guide in future discussions such that
    +    our codebase becomes more consistent over time.
    +
    +    Note that the intent is not to rename all functions which violate these
    +    semantics right away. Rather, the intent is to slowly converge towards a
    +    common style over time.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ Documentation/CodingGuidelines: For C programs:
      	void reset_strbuf(struct strbuf *buf);
      
     + - There are several common idiomatic names for functions performing
    -+   specific tasks on structures:
    ++   specific tasks on a structure `S`:
     +
    -+    - `<struct>_init()` initializes a structure without allocating the
    ++    - `S_init()` initializes a structure without allocating the
     +      structure itself.
     +
    -+    - `<struct>_release()` releases a structure's contents without
    -+      freeing the structure.
    ++    - `S_release()` releases a structure's contents without freeing the
    ++      structure.
    ++
    ++    - `S_clear()` is equivalent to `S_release()` followed by `S_init()`
    ++      such that the structure is directly usable after clearing it. When
    ++      `S_clear()` is provided, `S_init()` shall not allocate resources
    ++      that need to be released again.
     +
    -+    - `<struct>_free()` releases a structure's contents and frees the
    ++    - `S_free()` releases a structure's contents and frees the
     +      structure.
     +
      For Perl programs:
-:  ---------- > 5:  8789323ac7 Documentation: consistently use spaces inside initializers

In [1], we have improved our clang-format configuration to also specify the style for how to indent preprocessor directives. But while we have settled the question of where to put the indentation, either before or after the hash sign, we didn't specify exactly how to indent.

With the current configuration, clang-format uses tabs to indent each level of nested preprocessor directives, which is in fact unintentional and never done in our codebase. Instead, we use a mixture of indenting by either one or two spaces, where using a single space is somewhat more common.

Adapt our clang-format configuration accordingly by specifying an indentation width of one space.

[1]: <20240708092317.267915-1-karthik.188@gmail.com>

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .clang-format | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patrick Steinhardt <ps@pks.im> writes:

I totally missed this, thanks for fixing it up. The patch looks good.

In the preceding commit, we have settled on using a single space per nesting level to indent preprocessor directives. Clarify our coding guidelines accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/CodingGuidelines | 10 ++++++++++
 1 file changed, 10 insertions(+)

We nowadays have a proper mishmash of struct-related functions that are called `<verb>_<struct>` (e.g. `clear_prio_queue()`) versus functions that are called `<struct>_<verb>` (e.g. `strbuf_clear()`). While the former style may be easier to tie into a spoken conversation, most of our communication happens in text anyway. Furthermore, prefixing functions with the name of the structure they operate on makes it way easier to group them together, see which functions are related, and will also help folks who are using code completion.

Let's thus settle on one style, namely the one where functions start with the name of the structure they operate on.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/CodingGuidelines | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

We semi-regularly have discussions around whether a function shall be named `S_release()`, `S_clear()` or `S_free()`. Indeed, it may not be obvious which of these is preferable as we never really defined what each of these variants means exactly.

Carve out a space where we can add idiomatic names for common functions in our coding guidelines and define each of those functions. Like this, we can get to a shared understanding of their respective semantics and can easily point towards our style guide in future discussions such that our codebase becomes more consistent over time.

Note that the intent is not to rename all functions which violate these semantics right away. Rather, the intent is to slowly converge towards a common style over time.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/CodingGuidelines | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Our coding guide is inconsistent with how it uses spaces inside of initializers (`struct foo bar = { something }`). While we mostly carry the space between open and closing braces and the initialized members, in one case we don't.

Fix this one instance such that we consistently carry the space. This is also consistent with how clang-format formats such initializers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/CodingGuidelines | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patrick Steinhardt <ps@pks.im> writes:

Nothing jumps out at me as wrong/missing in this version.

Will queue.  Thanks.

Patrick Steinhardt <ps@pks.im> writes:

This series seems good. I did a read-through and have no changes to suggest! I'm really happy with these fixes which slowly improve the quality of the codebase.

Thanks!