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

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]

On Sun, Oct 07, 2007 at 04:07:07PM +0000, Alex Riesen wrote:
> 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.

  Then you can't write a filter, because you need to reallocate the
buffer before even starting to read the input buffer sometimes. If you
reallocate the strbuf, and that your original buffer was in there, you
lose.

> >   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")

  I'm totally open to better alternatives. We could probably easily get
rid of strbuf_end_filter, as whichever way to deal with the issue I try
to solve in a better way, in the end, it will always be just a "free".

  So, maybe there is a way to rename strbuf_start_filter so that it's
more straightforward. The way to use the API is:

 @  char *to_free = NULL;
 @  if ((src is inside dst && need_realloc) || operation is not in-place)
 @      to_free = strbuf_detach(dst, NULL);
 @  strbuf_make_room(dst, needed_size);

    // do whatever you want with src and dst

    free(to_free);

strbuf_start_filter tries to implement the block marked with `@'.  Of
course:
  * need_realloc == (needed_size >= dst->alloc)
  * src is inside dst means:
    src > dst->buf && src < dst->buf + dst->alloc
Though, those are both things that I find ugly to "know" in convert.c.
How things are allocated in strbufs is one of the few things we don't
want to assume anywhere outside of the strbuf module.

> > > 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

  Why check it ? You don't have to check. You have to keep it until
you're done with "src". Whichever the result was. The return of
strbuf_start_filter intends to stash a pointer to be freed for the cases
where "src" points into the destination buffer.

> >   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?

  I missed where having this live in convert.c rather than in strbuf.c
makes it less ugly or better in any sense.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]