{"thread":{"id":"10163","subject":"strbuf `filter' API","startedAt":"2007-10-05T20:19:29Z","lastAt":"2007-10-08T18:48:31Z","messageCount":8,"participants":["Pierre Habouzit","Alex Riesen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"54999","messageId":"1191615571-15946-1-git-send-email-madcoder@debian.org","threadId":"10163","inReplyTo":null,"subject":"strbuf `filter' API","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T20:19:29Z","receivedAt":"2007-10-05T20:19:29Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"  Ooookay,\n\n  I wasn't more thrilled about the previous patch than linus is. Here is\na possible API to hide the muck in the strbuf.c module, and have a way\nnicer to read code in convert.c.\n\n  It remais as efficient as the previous code, tries to reuse the buffer\nas much as possible to avoid reallocations, and deal with even more\ncases (it's possible to have the \"source\" buffer be inside the\ndestination strbuf now, which could sometimes help a lot).\n\n  I based this on top of master (because it's where my previous patch\nwas) but it should apply on next properly. It depends upon the previous\npatch though.\n\n  I'd say that the previous patch should go in master ASAP, and that\nthose two could be considered for next (having merged master in it first\nbtw).\n\nCheers,\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"55013","messageId":"20071006090621.GB2711@steel.home","threadId":"10163","inReplyTo":"1191615571-15946-2-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 1/2] Have a filter_start/filter_end API.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-10-06T09:06:21Z","receivedAt":"2007-10-06T09:06:21Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Pierre Habouzit, Fri, Oct 05, 2007 22:19:30 +0200:\n> Those are helpers to build functions that transform a buffer into a\n> strbuf, allowing the \"buffer\" to be taken from the destination buffer.\n\nThey are horrible. And very specialized for these \"filter\" routines.\nTo the point where I would move them into the file where they are used\n(convert.c only, isn't it?)\n\nIf you continue to insist the code is generic enough to justify its\nresidence in strbuf.c, continue reading.\n\nFirst off, what was wrong with dumb\n\n    void strbuf_make_room(struct strbuf *, size_t newsize);\n\nagain?\n\n> +void *strbuf_start_filter(struct strbuf *sb, const char *src, ssize_t hint)\n> +{\n> +\tif (src < sb->buf || src >= sb->buf + sb->alloc) {\n> +\t\tif (hint > 0 && (size_t)hint >= sb->alloc)\n> +\t\t\tstrbuf_grow(sb, hint - sb->len);\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\tif (hint < 0)\n> +\t\treturn strbuf_detach(sb, NULL);\n\nwhat is that for? Why can't the caller just use strbuf_detach? (He\nalready has to pass negative hint somehow, which should be a concious\naction).\n\n> +\t\t\n\ntrailing space (two HTs)\n\n> +\tif ((size_t)hint >= sb->alloc) {\n> +\t\tvoid *tmp = strbuf_detach(sb, NULL);\n> +\t\tstrbuf_grow(sb, hint);\n> +\t\treturn tmp;\n> +\t}\n> +\n> +\treturn NULL;\n> +}\n\nHow can one know when it sb is safe to use after strbuf_end_filter?\nIt is for the first \"if\", for example. free() wont free the buf in sb.\nOh, right, one can check if returned pointer !NULL. Which just adds\nmore code to handle your API.\n\nWhat actually happens to sb? Is it detached? Is it reallocated?\nWhen it is detached and when it is reallocated?\n\nWhy is the returned pointer useful only for giving it to\nstrbuf_end_filter?\n\nTake for example your change in crlf_to_git:\n@@ -85,6 +85,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n {\n \tstruct text_stat stats;\n \tchar *dst;\n+\tvoid *tmp;\n \n \tif ((action == CRLF_BINARY) || !auto_crlf || !len)\n \t\treturn 0;\n@@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n-\t/* only grow if not in place */\n-\tif (strbuf_avail(buf) + buf->len < len)\n-\t\tstrbuf_grow(buf, len - buf->len);\n+\ttmp = strbuf_start_filter(buf, src, len);\n \tdst = buf->buf;\n \tif (action == CRLF_GUESS) {\n \t\t/*\n@@ -133,13 +132,14 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t} while (--len);\n \t}\n \tstrbuf_setlen(buf, dst - buf->buf);\n+\tstrbuf_end_filter(tmp);\n \treturn 1;\n }\n\nAnd try to rewrite it with the strbuf_make_room:\n\n@@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n-\t/* only grow if not in place */\n-\tif (strbuf_avail(buf) + buf->len < len)\n-\t\tstrbuf_grow(buf, len - buf->len);\n+\tstrbuf_make_room(buf, len);\n \tdst = buf->buf;\n \tif (action == CRLF_GUESS) {\n \t\t/*\n\nThe change looks rather simple\n\n> +/*----- filter API -----*/\n> +extern void *strbuf_start_filter(struct strbuf *, const char *, ssize_t);\n> +extern void strbuf_end_filter(void *p);\n\nI find the naming very confusing: what filtering takes place where?\nIf strbuf_end_filter is just free, why is it needed at all? For the\nsake of wrapping free()?\n"},{"id":"55052","messageId":"20071007145355.GC10024@artemis.corp","threadId":"10163","inReplyTo":"20071006090621.GB2711@steel.home","subject":"Re: [PATCH 1/2] Have a filter_start/filter_end API.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-07T14:53:55Z","receivedAt":"2007-10-07T14:53:55Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sat, Oct 06, 2007 at 09:06:21AM +0000, Alex Riesen wrote:\n> Pierre Habouzit, Fri, Oct 05, 2007 22:19:30 +0200:\n> > Those are helpers to build functions that transform a buffer into a\n> > strbuf, allowing the \"buffer\" to be taken from the destination buffer.\n> \n> They are horrible. And very specialized for these \"filter\" routines.\n> To the point where I would move them into the file where they are used\n> (convert.c only, isn't it?)\n\n  For now they are, but moving them into convert.c is at the very least\nawkward.\n\n> If you continue to insist the code is generic enough to justify its\n> residence in strbuf.c, continue reading.\n>\n> First off, what was wrong with dumb\n>\n>     void strbuf_make_room(struct strbuf *, size_t newsize);\n>\n> again?\n\n  If newsize is >= sb->alloc then the area is reallocated, the pointer\nmay move, and the \"src\" pointer would then be invalidated.\n\n> > +void *strbuf_start_filter(struct strbuf *sb, const char *src, ssize_t hint)\n> > +{\n> > +\tif (src < sb->buf || src >= sb->buf + sb->alloc) {\n> > +\t\tif (hint > 0 && (size_t)hint >= sb->alloc)\n> > +\t\t\tstrbuf_grow(sb, hint - sb->len);\n> > +\t\treturn NULL;\n> > +\t}\n> > +\n> > +\tif (hint < 0)\n> > +\t\treturn strbuf_detach(sb, NULL);\n\n> what is that for? Why can't the caller just use strbuf_detach? (He\n> already has to pass negative hint somehow, which should be a concious\n> action).\n\n  The idea is to have a unified API to deal with both the cases where\nthe filtering is known not to work in place by the caller, or for the\ncases where it could if enough space is allocated but that a realloc is\nneeded.\n\n\n> > +\tif ((size_t)hint >= sb->alloc) {\n> > +\t\tvoid *tmp = strbuf_detach(sb, NULL);\n> > +\t\tstrbuf_grow(sb, hint);\n> > +\t\treturn tmp;\n> > +\t}\n> > +\n> > +\treturn NULL;\n> > +}\n>\n> How can one know when it sb is safe to use after strbuf_end_filter?\n\n  We could document it, that's not an issue.\n\n> It is for the first \"if\", for example. free() wont free the buf in sb.\n> Oh, right, one can check if returned pointer !NULL. Which just adds\n> more code to handle your API.\n\n  I don't get that part. free(NULL) is totally ok.\n\n> What actually happens to sb? Is it detached? Is it reallocated?\n> When it is detached and when it is reallocated?\n\n  It is detached if the filter does not works in place (caller says that\nwith '-1' as a hint) or if it works in place but needs a buffer\nreallocation.\n\n> Why is the returned pointer useful only for giving it to\n> strbuf_end_filter?\n\n  Because the filter works on a {src, len} buffer, that points into the\nbuffer that we must free later in strbuf_end_filter.\n\n> Take for example your change in crlf_to_git:\n> @@ -85,6 +85,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n>  {\n>  \tstruct text_stat stats;\n>  \tchar *dst;\n> +\tvoid *tmp;\n>  \n>  \tif ((action == CRLF_BINARY) || !auto_crlf || !len)\n>  \t\treturn 0;\n> @@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n>  \t\t\treturn 0;\n>  \t}\n>  \n> -\t/* only grow if not in place */\n> -\tif (strbuf_avail(buf) + buf->len < len)\n> -\t\tstrbuf_grow(buf, len - buf->len);\n> +\ttmp = strbuf_start_filter(buf, src, len);\n>  \tdst = buf->buf;\n>  \tif (action == CRLF_GUESS) {\n>  \t\t/*\n> @@ -133,13 +132,14 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n>  \t\t} while (--len);\n>  \t}\n>  \tstrbuf_setlen(buf, dst - buf->buf);\n> +\tstrbuf_end_filter(tmp);\n>  \treturn 1;\n>  }\n> \n> And try to rewrite it with the strbuf_make_room:\n> \n> @@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n>  \t\t\treturn 0;\n>  \t}\n>  \n> -\t/* only grow if not in place */\n> -\tif (strbuf_avail(buf) + buf->len < len)\n> -\t\tstrbuf_grow(buf, len - buf->len);\n> +\tstrbuf_make_room(buf, len);\n>  \tdst = buf->buf;\n>  \tif (action == CRLF_GUESS) {\n>  \t\t/*\n> \n> The change looks rather simple\n\n  but is wrong because you may just break \"src\". It's not the case here\nbecause len always fits, but it's IMHO the kind of knowledge of the\ninternals of strbufs we should not rely on.\n\n> > +/*----- filter API -----*/\n> > +extern void *strbuf_start_filter(struct strbuf *, const char *, ssize_t);\n> > +extern void strbuf_end_filter(void *p);\n> \n> I find the naming very confusing: what filtering takes place where?\n> If strbuf_end_filter is just free, why is it needed at all? For the\n> sake of wrapping free()?\n\n  Those are not filters, but help writing filters that would have this\nprototype:\n\n  int some_filter(const char *src, size_t len, struct strbuf *dst);\n\n  Allowing [src..src+len[ to be a subpart of `dst'. The usual semantics\nof such filters is that it returns 0 if it did nothing, 1 if it wrote in\ndst.\n\n  This way, you can write filter_all being filter1, filter2, filter3\ndone one after the other this way:\n\nint filter_all(const char *src, size_t len, struct strbuf *dst) {\n    int res = 0;\n\n    res |= filter1(src, len, dst);\n    if (res) {\n        src = dst->buf;\n        len = dst->len;\n    }\n    res |= filter2(src, len, dst);\n    if (res) {\n        src = dst->buf;\n        len = dst->len;\n    }\n    return res | filter3(src, len, dst);\n}\n\n  Note that I did not created this semantics, it was how convert.c\nworked already, in a even more convoluted way before.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"55057","messageId":"20071007160707.GA3270@steel.home","threadId":"10163","inReplyTo":"20071007145355.GC10024@artemis.corp","subject":"Re: [PATCH 1/2] Have a filter_start/filter_end API.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-10-07T16:07:07Z","receivedAt":"2007-10-07T16:07:07Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Pierre Habouzit, Sun, Oct 07, 2007 16:53:55 +0200:\n> On Sat, Oct 06, 2007 at 09:06:21AM +0000, Alex Riesen wrote:\n> > If you continue to insist the code is generic enough to justify its\n> > residence in strbuf.c, continue reading.\n> >\n> > First off, what was wrong with dumb\n> >\n> >     void strbuf_make_room(struct strbuf *, size_t newsize);\n> >\n> > again?\n> \n>   If newsize is >= sb->alloc then the area is reallocated, the pointer\n> may move, and the \"src\" pointer would then be invalidated.\n\nSo what? You already _have_ to know it points inside the strbuf, so\nyou can't expect it to be valid after any serious strbuf operation.\n\n> > what is that for? Why can't the caller just use strbuf_detach? (He\n> > already has to pass negative hint somehow, which should be a concious\n> > action).\n> \n>   The idea is to have a unified API to deal with both the cases where\n> the filtering is known not to work in place by the caller, or for the\n> cases where it could if enough space is allocated but that a realloc is\n> needed.\n\nthis just makes it convoluted and opaque (as in \"not transparent\")\n\n> > > +\tif ((size_t)hint >= sb->alloc) {\n> > > +\t\tvoid *tmp = strbuf_detach(sb, NULL);\n> > > +\t\tstrbuf_grow(sb, hint);\n> > > +\t\treturn tmp;\n> > > +\t}\n> > > +\n> > > +\treturn NULL;\n> > > +}\n> >\n> > How can one know when it sb is safe to use after strbuf_end_filter?\n> \n>   We could document it, that's not an issue.\n\nThe fact that you _have_to_ document is.\n\n> > It is for the first \"if\", for example. free() wont free the buf in sb.\n> > Oh, right, one can check if returned pointer !NULL. Which just adds\n> > more code to handle your API.\n> \n>   I don't get that part. free(NULL) is totally ok.\n\nNot that. One have to store the result of start_filter and check it\n\n> > What actually happens to sb? Is it detached? Is it reallocated?\n> > When it is detached and when it is reallocated?\n> \n>   It is detached if the filter does not works in place (caller says that\n> with '-1' as a hint) or if it works in place but needs a buffer\n> reallocation.\n\nToo many if's. Ugly\n\n>   Note that I did not created this semantics, it was how convert.c\n> worked already, in a even more convoluted way before.\n\nAnd why shouldn't these semantics kept to convert.c?\n"},{"id":"55069","messageId":"20071007165218.GE10024@artemis.corp","threadId":"10163","inReplyTo":"20071007160707.GA3270@steel.home","subject":"Re: [PATCH 1/2] Have a filter_start/filter_end API.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-07T16:52:18Z","receivedAt":"2007-10-07T16:52:18Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Oct 07, 2007 at 04:07:07PM +0000, Alex Riesen wrote:\n> Pierre Habouzit, Sun, Oct 07, 2007 16:53:55 +0200:\n> > On Sat, Oct 06, 2007 at 09:06:21AM +0000, Alex Riesen wrote:\n> > > If you continue to insist the code is generic enough to justify its\n> > > residence in strbuf.c, continue reading.\n> > >\n> > > First off, what was wrong with dumb\n> > >\n> > >     void strbuf_make_room(struct strbuf *, size_t newsize);\n> > >\n> > > again?\n> > \n> >   If newsize is >= sb->alloc then the area is reallocated, the pointer\n> > may move, and the \"src\" pointer would then be invalidated.\n> \n> So what? You already _have_ to know it points inside the strbuf, so\n> you can't expect it to be valid after any serious strbuf operation.\n\n  Then you can't write a filter, because you need to reallocate the\nbuffer before even starting to read the input buffer sometimes. If you\nreallocate the strbuf, and that your original buffer was in there, you\nlose.\n\n> >   The idea is to have a unified API to deal with both the cases where\n> > the filtering is known not to work in place by the caller, or for the\n> > cases where it could if enough space is allocated but that a realloc is\n> > needed.\n>\n> this just makes it convoluted and opaque (as in \"not transparent\")\n\n  I'm totally open to better alternatives. We could probably easily get\nrid of strbuf_end_filter, as whichever way to deal with the issue I try\nto solve in a better way, in the end, it will always be just a \"free\".\n\n  So, maybe there is a way to rename strbuf_start_filter so that it's\nmore straightforward. The way to use the API is:\n\n @  char *to_free = NULL;\n @  if ((src is inside dst && need_realloc) || operation is not in-place)\n @      to_free = strbuf_detach(dst, NULL);\n @  strbuf_make_room(dst, needed_size);\n\n    // do whatever you want with src and dst\n\n    free(to_free);\n\nstrbuf_start_filter tries to implement the block marked with `@'.  Of\ncourse:\n  * need_realloc == (needed_size >= dst->alloc)\n  * src is inside dst means:\n    src > dst->buf && src < dst->buf + dst->alloc\nThough, those are both things that I find ugly to \"know\" in convert.c.\nHow things are allocated in strbufs is one of the few things we don't\nwant to assume anywhere outside of the strbuf module.\n\n> > > It is for the first \"if\", for example. free() wont free the buf in sb.\n> > > Oh, right, one can check if returned pointer !NULL. Which just adds\n> > > more code to handle your API.\n> >\n> >   I don't get that part. free(NULL) is totally ok.\n>\n> Not that. One have to store the result of start_filter and check it\n\n  Why check it ? You don't have to check. You have to keep it until\nyou're done with \"src\". Whichever the result was. The return of\nstrbuf_start_filter intends to stash a pointer to be freed for the cases\nwhere \"src\" points into the destination buffer.\n\n> >   Note that I did not created this semantics, it was how convert.c\n> > worked already, in a even more convoluted way before.\n>\n> And why shouldn't these semantics kept to convert.c?\n\n  I missed where having this live in convert.c rather than in strbuf.c\nmakes it less ugly or better in any sense.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"55094","messageId":"20071007215041.GB2765@steel.home","threadId":"10163","inReplyTo":"20071007165218.GE10024@artemis.corp","subject":"Re: [PATCH 1/2] Have a filter_start/filter_end API.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-10-07T21:50:41Z","receivedAt":"2007-10-07T21:50:41Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Pierre Habouzit, Sun, Oct 07, 2007 18:52:18 +0200:\n>   So, maybe there is a way to rename strbuf_start_filter so that it's\n> more straightforward. The way to use the API is:\n> \n>  @  char *to_free = NULL;\n>  @  if ((src is inside dst && need_realloc) || operation is not in-place)\n>  @      to_free = strbuf_detach(dst, NULL);\n>  @  strbuf_make_room(dst, needed_size);\n\nWhy do you have to play these games with src being inside/outside?\nDon't you know where src already is? AFAICS in convert.c crlf_to_git -\nyou do. And everywhere else you know this fact too.\n\n>     // do whatever you want with src and dst\n> \n>     free(to_free);\n> \n> strbuf_start_filter tries to implement the block marked with `@'.  Of\n> course:\n>   * need_realloc == (needed_size >= dst->alloc)\n>   * src is inside dst means:\n>     src > dst->buf && src < dst->buf + dst->alloc\n\nin convert.c it is never \"inside\". It is _exactly_ the ->buf.\nNo need for generic \"inside\" check. Just test if src == ->buf.\n\nAnd AFAICS, there is no other users of the function.\n\n> Though, those are both things that I find ugly to \"know\" in convert.c.\n> How things are allocated in strbufs is one of the few things we don't\n> want to assume anywhere outside of the strbuf module.\n\nsrc is outside of strbuf scope. It is not internal to struct strbuf.\nThe caller must already know if it is inside of the given strbuf\ninstance.\n\nneed_realloc is covered by make_room, isn't it?\n\nI'd suggest just fix the caller, it is simple in convert.c: just use\nret, which contains exactly this information. If you insist on editing\nin-place, which makes your routines really need the in-placeability\ninformaion. Just give it to them, better explicitely. All of this\nmakes the routines very convert.c specific, which is the reason why I\nargument to have them just there and nowhere else.\n\nAlternatively, one can memdup ->buf (as it is the input for next\nfilter) every time a filter modifies it (which is safe, but simple,\nslow, requires memory, and may fragment heap):\n\n/* simplified */\nint convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)\n{\n\tint crlf = CRLF_GUESS;\n\tint ident = 0, ret = 0;\n\tchar *filter = NULL;\n\tconst char *tmp = src;\n\n\tret |= apply_filter(path, tmp, len, dst, filter);\n\tif (ret) {\n\t\ttmp = xmemdupz(dst->buf, dst->len);\n\t\tlen = dst->len;\n\t}\n\n\tif (crlf_to_git(path, tmp, len, dst, crlf)) {\n\t\tret |= 1;\n\t\tif (tmp != src)\n\t\t\tfree((void*)tmp);\n\t\ttmp = xmemdupz(dst->buf, dst->len);\n\t\tlen = dst->len;\n\t}\n\n\tret |= ident_to_git(path, tmp, len, dst, ident);\n\tif (tmp != src)\n\t\tfree((void*)tmp);\n\treturn ret;\n}\n\nIt is just to show that nothing like strbuf_start_filter is _needed_\nat all.\n"},{"id":"55167","messageId":"20071008072947.GB22552@artemis.corp","threadId":"10163","inReplyTo":"20071007215041.GB2765@steel.home","subject":"Re: [PATCH 1/2] Have a filter_start/filter_end API.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-08T07:29:47Z","receivedAt":"2007-10-08T07:29:47Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Oct 07, 2007 at 09:50:41PM +0000, Alex Riesen wrote:\n> Pierre Habouzit, Sun, Oct 07, 2007 18:52:18 +0200:\n> > Though, those are both things that I find ugly to \"know\" in convert.c.\n> > How things are allocated in strbufs is one of the few things we don't\n> > want to assume anywhere outside of the strbuf module.\n>\n> src is outside of strbuf scope. It is not internal to struct strbuf.\n> The caller must already know if it is inside of the given strbuf\n> instance.\n>\n> need_realloc is covered by make_room, isn't it?\n\n  Internally yes, but make_room may move the buffer, if that happens,\nthere is nothing we can do, in the case where we point inside (or at the\nbegining of - fwiw it's the same here) the buffer\n\n> I'd suggest just fix the caller, it is simple in convert.c: just use\n> ret, which contains exactly this information. If you insist on editing\n> in-place, which makes your routines really need the in-placeability\n> informaion. Just give it to them, better explicitely. All of this\n> makes the routines very convert.c specific, which is the reason why I\n> argument to have them just there and nowhere else.\n> \n> Alternatively, one can memdup ->buf (as it is the input for next\n> filter) every time a filter modifies it (which is safe, but simple,\n> slow, requires memory, and may fragment heap):\n\n  This is exactly what we are trying to avoid with the current form.\nGiven how you try to micro-optimize strbuf_cmp I'm a bit lost here…\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"55200","messageId":"20071008184831.GB3123@steel.home","threadId":"10163","inReplyTo":"20071008072947.GB22552@artemis.corp","subject":"Re: [PATCH 1/2] Have a filter_start/filter_end API.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-10-08T18:48:31Z","receivedAt":"2007-10-08T18:48:31Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Pierre Habouzit, Mon, Oct 08, 2007 09:29:47 +0200:\n> On Sun, Oct 07, 2007 at 09:50:41PM +0000, Alex Riesen wrote:\n> > Pierre Habouzit, Sun, Oct 07, 2007 18:52:18 +0200:\n> > > Though, those are both things that I find ugly to \"know\" in convert.c.\n> > > How things are allocated in strbufs is one of the few things we don't\n> > > want to assume anywhere outside of the strbuf module.\n> >\n> > src is outside of strbuf scope. It is not internal to struct strbuf.\n> > The caller must already know if it is inside of the given strbuf\n> > instance.\n> >\n> > need_realloc is covered by make_room, isn't it?\n> \n>   Internally yes, but make_room may move the buffer, if that happens,\n> there is nothing we can do, in the case where we point inside (or at the\n> begining of - fwiw it's the same here) the buffer\n\nupdate the outside pointer you can. But I actually lost all interest:\npersonally I never use the filters and deeply despise the reasons\ncaused their existence.\n\n> > I'd suggest just fix the caller, it is simple in convert.c: just use\n> > ret, which contains exactly this information. If you insist on editing\n> > in-place, which makes your routines really need the in-placeability\n> > informaion. Just give it to them, better explicitely. All of this\n> > makes the routines very convert.c specific, which is the reason why I\n> > argument to have them just there and nowhere else.\n> > \n> > Alternatively, one can memdup ->buf (as it is the input for next\n> > filter) every time a filter modifies it (which is safe, but simple,\n> > slow, requires memory, and may fragment heap):\n> \n>   This is exactly what we are trying to avoid with the current form.\n\nI take this suggestion back. Do not use memdup, as it is slow,\nrequires lots of memory and may fragment heap. Sorry for repeating.\n\n> Given how you try to micro-optimize strbuf_cmp I'm a bit lost here…\n> \n\nI didn't. It just happened. If I _wanted_ to optimize anything I\nwouldn't start with a function which is used exactly one time in a\nprogram with a name like \"rerere\" which is not even used in default\nconfiguration.\n"}]}