Re: [PATCH] Strbuf documentation: document most functions

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 3, 2008, 00:00 UTC
Message-ID: <alpine.DEB.1.00.0806030048010.13507@racer.site.net>
In-Reply-To: <1212447591-4870-1-git-send-email-vmiklos@frugalware.org>

Hi,

I am no author of strbuf, but hey, I thought I'd just give you a few comments...

In general, I'd rather leave the "->" from the members, since you have many instances where you access them with ".".

On Tue, 3 Jun 2008, Miklos Vajna wrote:

Show 11 quoted lines

> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> index a52e4f3..3879e0e 100644
> --- a/Documentation/technical/api-strbuf.txt
> +++ b/Documentation/technical/api-strbuf.txt
> @@ -1,6 +1,175 @@
>  strbuf API
>  ==========
>  
> -Talk about <strbuf.h>
> +strbuf's can be use in many ways: as a byte array, or to store arbitrary
> +long, overflow safe strings.

I think that you should not suggest using strbufs as byte array, even if that is certainly possible. Rather, you should say something like:

	An strbuf is NUL terminated for convenience, but no function in 
	the strbuf API actually relies on the string being free of NULs.

> +strbufs has some invariants that are very important to keep in mind:
> +
> +. The `->buf` member is always malloc-ed, hence strbuf's can be used to
> +  build complex strings/buffers whose final size isn't easily known.

Is this true?  I thought the initial string is empty, but not alloc'ed.

So I'd rather have something like

	The "buf" member is never NULL, so you can safely strcmp() it.

I'd like to see a comment that strbuf's _have_ to be initialized either by strbuf_init() or by "= STRBUF_INIT" before the invariants, though.

> +`strbuf_attach`::
> +
> +	Attaches a string to a buffer. You should specify the string to attach,
> +	the current length of the string and the amount of allocated memory.

... This string _must_ be malloc()ed, and after attaching, the pointer cannot be relied upon anymore, and neither be free()d directly.

> +`strbuf_read`::
> +
> +	Read the contents of a given file descriptor. The third argument can be
> +	used to give a hint about the file, to avoid reallocs.

s/about the file/& size/

> +`strbuf_read_file`::
> +
> +	Read the contents of a file, specified by its path. The third argument
> +	can be used to give a hint about the file, to avoid reallocs.

Ditto.

> +`strbuf_getline`::
> +
> +	Read a line from a FILE* pointer. The second argument specifies the line
> +	terminator character, like `'\n'`.

s/like/typically/

Thanks, Dscho