Re: [PATCH 1/2] Have a filter_start/filter_end API.

[Alex Riesen <raa.lkml@gmail.com>]

Pierre Habouzit, Sun, Oct 07, 2007 16:53:55 +0200:
> On Sat, Oct 06, 2007 at 09:06:21AM +0000, Alex Riesen wrote:
> > If you continue to insist the code is generic enough to justify its
> > residence in strbuf.c, continue reading.
> >
> > First off, what was wrong with dumb
> >
> >     void strbuf_make_room(struct strbuf *, size_t newsize);
> >
> > again?
> 
>   If newsize is >= sb->alloc then the area is reallocated, the pointer
> may move, and the "src" pointer would then be invalidated.

So what? You already _have_ to know it points inside the strbuf, so
you can't expect it to be valid after any serious strbuf operation.

> > what is that for? Why can't the caller just use strbuf_detach? (He
> > already has to pass negative hint somehow, which should be a concious
> > action).
> 
>   The idea is to have a unified API to deal with both the cases where
> the filtering is known not to work in place by the caller, or for the
> cases where it could if enough space is allocated but that a realloc is
> needed.

this just makes it convoluted and opaque (as in "not transparent")

> > > +	if ((size_t)hint >= sb->alloc) {
> > > +		void *tmp = strbuf_detach(sb, NULL);
> > > +		strbuf_grow(sb, hint);
> > > +		return tmp;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> >
> > How can one know when it sb is safe to use after strbuf_end_filter?
> 
>   We could document it, that's not an issue.

The fact that you _have_to_ document is.

> > It is for the first "if", for example. free() wont free the buf in sb.
> > Oh, right, one can check if returned pointer !NULL. Which just adds
> > more code to handle your API.
> 
>   I don't get that part. free(NULL) is totally ok.

Not that. One have to store the result of start_filter and check it

> > What actually happens to sb? Is it detached? Is it reallocated?
> > When it is detached and when it is reallocated?
> 
>   It is detached if the filter does not works in place (caller says that
> with '-1' as a hint) or if it works in place but needs a buffer
> reallocation.

Too many if's. Ugly

>   Note that I did not created this semantics, it was how convert.c
> worked already, in a even more convoluted way before.

And why shouldn't these semantics kept to convert.c?