Re: [PATCH] document string_list_clear

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 12, 2014 at 04:27:26PM -0500, Jeff King wrote:
> 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

>   [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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]