| Message ID | cover.1722323818.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Documentation: some coding guideline updates | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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. > ... > I think I captured everything that came out of the discussion, but > please let me know in case I misinterpreted or forgot anything. Nothing jumps out at me as wrong/missing in this version. Will queue. Thanks.
Patrick Steinhardt <ps@pks.im> writes: > 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! > 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!
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