Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()
- From
Jeff King <peff@peff.net>
- Date
- Jun 27, 2023, 08:54 UTC
- Message-ID
- <20230627085422.GJ1226768@coredump.intra.peff.net>
- In-Reply-To
- <150df0b0-0548-f291-2b68-960841dd1c97@web.de>
On Sat, Jun 17, 2023 at 10:43:17PM +0200, René Scharfe wrote:
> Avoid the overhead of passing context to a callback function of > strbuf_expand() by using strbuf_expand_step() in a loop instead. It > requires explicit handling of %% and unrecognized placeholders, but is > simpler, more direct and avoids void pointers.
I like this. I don't care that much about the runtime overhead of passing the context around, but if you meant by "overhead" the programmer time and confusion in stuffing everything into context structs, then I agree this is much better. :)
It does still feel like we should be handling "%%" on behalf of the callers.
Show 8 quoted lines
> builtin/cat-file.c | 35 +++++++-------- > builtin/ls-files.c | 109 +++++++++++++++++++-------------------------- > builtin/ls-tree.c | 107 +++++++++++++++++--------------------------- > daemon.c | 61 ++++++++----------------- > pretty.c | 72 ++++++++++++++++++------------ > strbuf.c | 20 --------- > strbuf.h | 37 ++------------- > 7 files changed, 169 insertions(+), 272 deletions(-)
The changes mostly looked OK to me (and the diffstat is certainly pleasing). The old callbacks returned a "consumed" length, and we need each "step" caller to now do "format += consumed" themselves. At first glance I thought there were cases where you didn't, but then I realized that you are relying on skip_prefix() to do that incrementing. Which makes sense and the resulting code looks nice, but it took me a minute to realize what was going on.
Show 27 quoted lines
> @@ -1894,7 +1880,26 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
> return;
> fmt = user_format;
> }
> - strbuf_expand(&dummy, fmt, userformat_want_item, w);
> + while (strbuf_expand_step(&dummy, &fmt)) {
> + if (skip_prefix(fmt, "%", &fmt))
> + continue;
> +
> + if (*fmt == '+' || *fmt == '-' || *fmt == ' ')
> + fmt++;
> +
> + switch (*fmt) {
> + case 'N':
> + w->notes = 1;
> + break;
> + case 'S':
> + w->source = 1;
> + break;
> + case 'd':
> + case 'D':
> + w->decorate = 1;
> + break;
> + }
> + }
> strbuf_release(&dummy);
> }This one actually doesn't increment the format (so we restart the expansion on "N" or whatever). But neither did the original! It always returned 0:
Show 23 quoted lines
> -static size_t userformat_want_item(struct strbuf *sb UNUSED,
> - const char *placeholder,
> - void *context)
> -{
> - struct userformat_want *w = context;
> -
> - if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')
> - placeholder++;
> -
> - switch (*placeholder) {
> - case 'N':
> - w->notes = 1;
> - break;
> - case 'S':
> - w->source = 1;
> - break;
> - case 'd':
> - case 'D':
> - w->decorate = 1;
> - break;
> - }
> - return 0;
> -}So probably OK, though a little funny.
It also feels like this whole function would be just as happy using "strchr()", since it throws away the expanded result anyway. But that can be for another time. :)
Show 15 quoted lines
> @@ -1912,7 +1917,16 @@ void repo_format_commit_message(struct repository *r,
> const char *output_enc = pretty_ctx->output_encoding;
> const char *utf8 = "UTF-8";
>
> - strbuf_expand(sb, format, format_commit_item, &context);
> + while (strbuf_expand_step(sb, &format)) {
> + size_t len;
> +
> + if (skip_prefix(format, "%", &format))
> + strbuf_addch(sb, '%');
> + else if ((len = format_commit_item(sb, format, &context)))
> + format += len;
> + else
> + strbuf_addch(sb, '%');
> + }This one doesn't advance the format for a not-understood placeholder. But that's OK, because we know it isn't "%", so starting the search from there again is correct.
Show 9 quoted lines
> @@ -1842,7 +1852,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ > } > > orig_len = sb->len; > - if (((struct format_commit_context *)context)->flush_type != no_flush) > + if ((context)->flush_type != no_flush) > consumed = format_and_pad_commit(sb, placeholder, context); > else > consumed = format_commit_one(sb, placeholder, context);
Since we're no longer casting, the extra parentheses seem redundant now. I.e., this can just be context->flush_type.
-Peff