| Message ID | 7f07bf1f3beee2f74a3572d2b9a8d28b6535053e.1721818488.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Documentation: some coding guideline updates | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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(+) > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > index 1d09384f28..34fcbcb5a4 100644 > --- a/Documentation/CodingGuidelines > +++ b/Documentation/CodingGuidelines > @@ -541,6 +541,25 @@ 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. > + Nit: It would be nice to add `<struct>_<verb>` here, since we do something similar in the next patch. > + struct strbuf; > + > + void strbuf_add(struct strbuf *buf, ...); > + > + void strbuf_reset(struct strbuf *buf); > + > + is preferred over: > + > + struct strbuf; > + > + void add_string(struct strbuf *buf, ...); > + 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 :) > + void reset_strbuf(struct strbuf *buf); > + > For Perl programs: > > - Most of the C guidelines above apply. > -- > 2.46.0.rc1.dirty
On Wed, Jul 24, 2024 at 04:42:29AM -0700, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > > index 1d09384f28..34fcbcb5a4 100644 > > --- a/Documentation/CodingGuidelines > > +++ b/Documentation/CodingGuidelines > > @@ -541,6 +541,25 @@ 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. > > + > > Nit: It would be nice to add `<struct>_<verb>` here, since we do something > similar in the next patch. Makes sense, will do. Patrick
Patrick Steinhardt <ps@pks.im> writes: > + - Functions that operate on a specific structure and which are used by > + other subsystems shall be named after the structure. 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). > + The function > + name should start with the name of the structure followed by a verb. > + E.g. > + > + struct strbuf; > + > + void strbuf_add(struct strbuf *buf, ...); > + > + void strbuf_reset(struct strbuf *buf); > + > + is preferred over: > + > + struct strbuf; > + > + void add_string(struct strbuf *buf, ...); > + > + void reset_strbuf(struct strbuf *buf); 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: > Patrick Steinhardt <ps@pks.im> writes: > >> + - 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. >> + >> + struct strbuf; >> + >> + void strbuf_add(struct strbuf *buf, ...); >> + ... >> + void strbuf_reset(struct strbuf *buf); 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: > Patrick Steinhardt <ps@pks.im> writes: > > > + - Functions that operate on a specific structure and which are used by > > + other subsystems shall be named after the structure. > > 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). 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. > > + The function > > + name should start with the name of the structure followed by a verb. > > + E.g. > > + > > + struct strbuf; > > + > > + void strbuf_add(struct strbuf *buf, ...); > > + > > + void strbuf_reset(struct strbuf *buf); > > + > > + is preferred over: > > + > > + struct strbuf; > > + > > + void add_string(struct strbuf *buf, ...); > > + > > + void reset_strbuf(struct strbuf *buf); > > Do we want to rename start_command(), finish_command(), > run_command() and pipe_command()? 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. > child_process_start() sounds somewhat ungrammatical. 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()`. > 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 *"). Yeah, that's fair indeed. Patrick
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 1d09384f28..34fcbcb5a4 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -541,6 +541,25 @@ 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. + + struct strbuf; + + void strbuf_add(struct strbuf *buf, ...); + + void strbuf_reset(struct strbuf *buf); + + is preferred over: + + struct strbuf; + + void add_string(struct strbuf *buf, ...); + + void reset_strbuf(struct strbuf *buf); + For Perl programs: - Most of the C guidelines above apply.
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(+)