Re: [PATCH 1/2] Have a filter_start/filter_end API.
On Sat, Oct 06, 2007 at 09:06:21AM +0000, Alex Riesen wrote:
Show 7 quoted lines
> Pierre Habouzit, Fri, Oct 05, 2007 22:19:30 +0200:
> > Those are helpers to build functions that transform a buffer into a
> > strbuf, allowing the "buffer" to be taken from the destination buffer.
>
> They are horrible. And very specialized for these "filter" routines.
> To the point where I would move them into the file where they are used
> (convert.c only, isn't it?)
For now they are, but moving them into convert.c is at the very least
awkward.
Show 8 quoted lines
> 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.
Show 10 quoted lines
> > +void *strbuf_start_filter(struct strbuf *sb, const char *src, ssize_t hint)
> > +{
> > + if (src < sb->buf || src >= sb->buf + sb->alloc) {
> > + if (hint > 0 && (size_t)hint >= sb->alloc)
> > + strbuf_grow(sb, hint - sb->len);
> > + return NULL;
> > + }
> > +
> > + if (hint < 0)
> > + return strbuf_detach(sb, NULL);> 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.
Show 10 quoted lines
> > + 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.
> 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.
> 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.
> Why is the returned pointer useful only for giving it to
> strbuf_end_filter?
Because the filter works on a {src, len} buffer, that points into the
buffer that we must free later in strbuf_end_filter.Show 43 quoted lines
> Take for example your change in crlf_to_git:
> @@ -85,6 +85,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> {
> struct text_stat stats;
> char *dst;
> + void *tmp;
>
> if ((action == CRLF_BINARY) || !auto_crlf || !len)
> return 0;
> @@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> return 0;
> }
>
> - /* only grow if not in place */
> - if (strbuf_avail(buf) + buf->len < len)
> - strbuf_grow(buf, len - buf->len);
> + tmp = strbuf_start_filter(buf, src, len);
> dst = buf->buf;
> if (action == CRLF_GUESS) {
> /*
> @@ -133,13 +132,14 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> } while (--len);
> }
> strbuf_setlen(buf, dst - buf->buf);
> + strbuf_end_filter(tmp);
> return 1;
> }
>
> And try to rewrite it with the strbuf_make_room:
>
> @@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> return 0;
> }
>
> - /* only grow if not in place */
> - if (strbuf_avail(buf) + buf->len < len)
> - strbuf_grow(buf, len - buf->len);
> + strbuf_make_room(buf, len);
> dst = buf->buf;
> if (action == CRLF_GUESS) {
> /*
>
> The change looks rather simple but is wrong because you may just break "src". It's not the case here
because len always fits, but it's IMHO the kind of knowledge of the
internals of strbufs we should not rely on.
Show 7 quoted lines
> > +/*----- filter API -----*/
> > +extern void *strbuf_start_filter(struct strbuf *, const char *, ssize_t);
> > +extern void strbuf_end_filter(void *p);
>
> I find the naming very confusing: what filtering takes place where?
> If strbuf_end_filter is just free, why is it needed at all? For the
> sake of wrapping free()?
Those are not filters, but help writing filters that would have this
prototype:
int some_filter(const char *src, size_t len, struct strbuf *dst);
Allowing [src..src+len[ to be a subpart of `dst'. The usual semantics
of such filters is that it returns 0 if it did nothing, 1 if it wrote in
dst.
This way, you can write filter_all being filter1, filter2, filter3
done one after the other this way:
int filter_all(const char *src, size_t len, struct strbuf *dst) {
int res = 0; res |= filter1(src, len, dst);
if (res) {
src = dst->buf;
len = dst->len;
}
res |= filter2(src, len, dst);
if (res) {
src = dst->buf;
len = dst->len;
}
return res | filter3(src, len, dst);
} Note that I did not created this semantics, it was how convert.c
worked already, in a even more convoluted way before.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
Pierre Habouzit