Re: [PATCH] document string_list_clear

From: Stefan Beller <sbeller@google.com>
Date: Dec 12, 2014, 22:32 UTC
Message-ID: <20141212223241.GA19972@google.com>
In-Reply-To: <20141212212726.GA26284@peff.net>

On Fri, Dec 12, 2014 at 04:27:26PM -0500, Jeff King wrote:

Show 20 quoted lines

> On Fri, Dec 12, 2014 at 11:35:52AM -0800, Jonathan Nieder wrote:
> 
> > If I were doing it, I would first de-asciidoc within technical/ and
> > then move into the header in a separate patch.  Or first move with
> > asciidoc intact and then de-asciidoc in a separate patch.  Combining
> > the two into a single patch is also fine.  Please don't change wording
> > at the same time.
> 
> Here's what I came up with. The first patch probably does more than
> you'd like (and more than I would have done if I were starting from
> scratch today). But I didn't want to get into undoing the stripping of
> each function-name list item that I showed earlier, as it would be a lot
> of tedious manual work. If we decide we want to keep those, I'm happy to
> do the work to restore them, but it didn't seem like a good tradeoff
> just to create an intermediate state to make the patch prettier.
> 
> I did split out some of the other formatting decisions, though, so they
> can be evaluated individually.
> 
>   [1/4]: strbuf: migrate api-strbuf.txt documentation to strbuf.h

(optional nit): The subject of this patch is slightly different than the others. How about: strbuf.h: integrate api-strbuf.txt documentation

Show 5 quoted lines

>   [2/4]: strbuf.h: drop asciidoc list formatting from API docs
>   [3/4]: strbuf.h: format asciidoc code blocks as 4-space indent
>   [4/4]: strbuf.h: reorganize api function grouping headers
> 
> -Peff

I have reviewed the series and I personally like it as I am more often in the learners/discovery stage than the fast lookup stage as Junio mentioned.

Another inconsistency I found specifically related to the strbuf.h documentation is the apparent changeing style.

At the beginning we have 
	extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);

Even with the documentation attached, which size_t is the length and what the amount of allocated memory? Later we have more explaining names, such as

extern struct strbuf **strbuf_split_buf(const char *, size_t,
					int terminator, int max);

Please append the following patch to your series. I noticed this as the text editor I use uses different colors for

	/** comments starting with double stars used with i.e. doxygen */
	/* random comments in the file */

Thanks, Stefan

-----------8<------------------
From 5d0f98aba65e8b1415ebfcd8e06b67203e9305a7 Mon Sep 17 00:00:00 2001
From: Stefan Beller <sbeller@google.com>
Date: Fri, 12 Dec 2014 14:27:15 -0800
Subject: [PATCH] strbuf.h: Unify documentation comments beginnings

We want to preserve the meaning of "/**" to start a documentary in depth comment, so all of the documenting comments should start with a "/**".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 strbuf.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 8649a0a..95d49e1 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -438,7 +438,7 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 		return 0;
 }
 
-/*
+/**
  * Split str (of length slen) at the specified terminator character.
  * Return a null-terminated array of pointers to strbuf objects
  * holding the substrings.  The substrings include the terminator,
@@ -454,7 +454,7 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 extern struct strbuf **strbuf_split_buf(const char *, size_t,
 					int terminator, int max);
 
-/*
+/**
  * Split a NUL-terminated string at the specified terminator
  * character.  See strbuf_split_buf() for more information.
  */
@@ -464,7 +464,7 @@ static inline struct strbuf **strbuf_split_str(const char *str,
 	return strbuf_split_buf(str, strlen(str), terminator, max);
 }
 
-/*
+/**
  * Split a strbuf at the specified terminator character.  See
  * strbuf_split_buf() for more information.
  */
@@ -474,7 +474,7 @@ static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,
 	return strbuf_split_buf(sb->buf, sb->len, terminator, max);
 }
 
-/*
+/**
  * Split a strbuf at the specified terminator character.  See
  * strbuf_split_buf() for more information.
  */
@@ -484,7 +484,7 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
 	return strbuf_split_max(sb, terminator, 0);
 }
 
-/*
+/**
  * Free a NULL-terminated list of strbufs (for example, the return
  * values of the strbuf_split*() functions).
  */
@@ -492,7 +492,7 @@ extern void strbuf_list_free(struct strbuf **);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
-/*
+/**
  * Append s to sb, with the characters '<', '>', '&' and '"' converted
  * into XML entities.
  */

-- 
2.2.0.38.g71d6cb7