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@xxxxxxxxx/
+ 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@xxxxxx>
@@ 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@xxxxxx>
## 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@xxxxxx>
## 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@xxxxxx>
@@ 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
--
2.46.0.dirty
Attachment:
signature.asc
Description: PGP signature
