Re: [PATCH 05/10] evolve: add the change-table structure

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 28, 2022, 15:14 UTC
Message-ID: <220928.86pmffwmft.gmgdl@evledraar.gmail.com>
In-Reply-To: <72c4e8c9-62bb-96aa-7b42-d7103b7873eb@dunelm.org.uk>

On Wed, Sep 28 2022, Phillip Wood wrote:

Show 34 quoted lines

> On 27/09/2022 16:28, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Sep 27 2022, Phillip Wood wrote:
>> 
>>> On 27/09/2022 14:50, Ævar Arnfjörð Bjarmason wrote:
>>>> On Tue, Sep 27 2022, Phillip Wood wrote:
>>>>
>>>>> On 23/09/2022 19:55, Stefan Xenos via GitGitGadget wrote:
>>>>>> +/**
>>>>>
>>>>> We tend to just use '/*' rather than '/**'
>>>> No, we use both, and /** is correct here. It's an API-doc syntax,
>>>> see
>>>> e.g. strbuf.h.
>>>> It's explicitly meant for this sort of thing, i.e. comments on
>>>> public
>>>> structs in a header & the functions in a header (and struct members,
>>>> etc.).
>>>
>>> We don't do that consistently, we don't mention them in
>>> CodingGuidelines and we don't use anything that processes API-doc
>>> comments. It would be a lot simpler and it would be consistent with
>>> our coding guidelines just to use the same style everywhere. That
>>> would avoid problems where this series uses API-doc comments for
>>> in-code comments in .c files and where single line comments in header
>>> files do not use the API-doc syntax.
>> Yes, this isn't documented in CodingGuidelines (but FWIW is in
>> various
>> commit messages).
>> I'm pointing out that this isn't a mistake, but the preferred style
>> for
>> new API docs.
>
> It seems a bit a stretch to call it the preferred style, however I had
> thought all our uses were historic but that's not the case.

FWIW that claim of mine comes from my (admittedly fuzzy recollection of) history, around the time that this was being worked out (e.g. where API docs should live, they used to mostly be in Documentation/technical/, now they're mostly in *.h) we ended up with this mention in CodingGudielines:

 - When you come up with an API, document its functions and structures
   in the header file that exposes the API to its callers. Use what is
   in "strbuf.h" as a model for the appropriate tone and level of
   detail.

I.e. at some point we decreed strbuf.h as a best-practice model to follow. I think every one of my own use of "/**" has come after reading that file...

But patches to make it more explicit are most welcome. FWIW I think I looked into that once, but couldn't find a canonical reference for what this syntax is even called.

Hrm, looking a bit more I see it's probably JavaDoc (at least Emacs highlights it as such). A grep of:

	git grep '^ \* @' -- '*.[ch]'

Shows that we make almost no use of the meta-syntax (i.e. "tags": https://en.wikipedia.org/wiki/Javadoc#Table_of_Javadoc_tags)

Show 12 quoted lines

> Chris if you want to use '/**' style comments for the API docs then
> please do so consistently and do not use them elsewhere.
>
>> At least Emacs knows how to highlight these differently, which is the
>> main use I personally get out of them, I don't know what other use-cases
>> there are for them...
>
> I've come across them in projects that use gtk-doc or other
> documentation generators where it is necessary to distinguish 
> 'documentation' from 'code comments'. I don't think they add much
> value if one is not generating documentation from the source, it is
> just one more thing for contributors to remember.

I for one find it very useful these "API docs" are shown differently in my editor, even if I'm not using some fancier (and hypothetical) "make javadoc" target.

I.e. you can ideally skim the *.h file and see at a glance what's an implementation comment v.s. API doc comment.

Except that even for the supposed role model of strbuf.h we forgot in some cases, and should have the below applied to it, oh well... :)

diff --git a/strbuf.h b/strbuf.h
index 76965a17d44..46549986ae3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -492,7 +492,7 @@ int strbuf_getline_lf(struct strbuf *sb, FILE *fp);
 /* Uses NUL as the line terminator */
 int strbuf_getline_nul(struct strbuf *sb, FILE *fp);
 
-/*
+/**
  * Similar to strbuf_getline_lf(), but additionally treats a CR that
  * comes immediately before the LF as part of the terminator.
  * This is the most friendly version to be used to read "text" files
@@ -610,7 +610,7 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
 	return strbuf_split_max(sb, terminator, 0);
 }
 
-/*
+/**
  * Adds all strings of a string list to the strbuf, separated by the given
  * separator.  For example, if sep is
  *   ', '
@@ -653,7 +653,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
 			   const char *const *env);
 
-/*
+/**
  * In contrast to `launch_editor()`, this function writes out the contents
  * of the specified file first, then clears the `buffer`, then launches
  * the editor and reads back in the file contents into the `buffer`.
@@ -693,7 +693,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 	strbuf_complete(sb, '\n');
 }
 
-/*
+/**
  * Copy "name" to "sb", expanding any special @-marks as handled by
  * interpret_branch_name(). The result is a non-qualified branch name
  * (so "foo" or "origin/master" instead of "refs/heads/foo" or
@@ -707,7 +707,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 void strbuf_branchname(struct strbuf *sb, const char *name,
 		       unsigned allowed);
 
-/*
+/**
  * Like strbuf_branchname() above, but confirm that the result is
  * syntactically valid to be used as a local branch name in refs/heads/.
  *