Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()
- From
René Scharfe <l.s.r@web.de>
- Date
- Jun 27, 2023, 16:31 UTC
- Message-ID
- <8d2223b8-ab33-be5a-20ea-ad60f6cbcc75@web.de>
- In-Reply-To
- <20230627085422.GJ1226768@coredump.intra.peff.net>
Am 27.06.23 um 10:54 schrieb Jeff King:
Show 11 quoted lines
> 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. :)
Indeed, I meant the burden of being forced to define a struct and filling in all necessary context. Bureaucratic overhead.
> It does still feel like we should be handling "%%" on behalf of the > callers.
I feel the same, but restrained myself from doing that, so that we can see all the pieces for now. It allows us to recombine them in better ways than before.
Show 16 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.
*nod* The "returns consumed length" style is still used by strbuf_expand_literal, though, so we have a bit of a mix. Which works, but a uniform convention might be better.
Show 61 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:
>
>> -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. :)Good idea! And the conversion to a loop brings us halfway there.
Show 20 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.Right. This is copied from strbuf_expand.
Show 13 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.
Indeed.
René