From: "René Scharfe" <l.s.r@web.de>
To: Jeff King <peff@peff.net>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()
Date: Tue, 27 Jun 2023 18:31:55 +0200 [thread overview]
Message-ID: <8d2223b8-ab33-be5a-20ea-ad60f6cbcc75@web.de> (raw)
In-Reply-To: <20230627085422.GJ1226768@coredump.intra.peff.net>
Am 27.06.23 um 10:54 schrieb Jeff King:
> 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.
>> 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.
>> @@ -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.
>
>> @@ -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.
>
>> @@ -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é
next prev parent reply other threads:[~2023-06-27 16:32 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-17 20:37 [PATCH 0/5] replace strbuf_expand() René Scharfe
2023-06-17 20:40 ` [PATCH 1/5] pretty: factor out expand_separator() René Scharfe
2023-06-17 20:41 ` [PATCH 2/5] strbuf: factor out strbuf_expand_step() René Scharfe
2023-06-19 16:10 ` Taylor Blau
2023-06-20 16:25 ` René Scharfe
2023-06-21 12:26 ` Taylor Blau
2023-06-27 8:26 ` Jeff King
2023-06-27 16:21 ` René Scharfe
2023-06-17 20:42 ` [PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step() René Scharfe
2023-06-27 8:29 ` Jeff King
2023-06-27 8:35 ` Jeff King
2023-06-27 16:24 ` René Scharfe
2023-06-17 20:43 ` [PATCH 4/5] replace strbuf_expand() " René Scharfe
2023-06-27 8:54 ` Jeff King
2023-06-27 16:31 ` René Scharfe [this message]
2023-06-27 20:19 ` Jeff King
2023-06-17 20:44 ` [PATCH 5/5] strbuf: simplify strbuf_expand_literal_cb() René Scharfe
2023-06-27 8:57 ` Jeff King
2023-06-27 16:32 ` René Scharfe
2023-06-27 20:21 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8d2223b8-ab33-be5a-20ea-ad60f6cbcc75@web.de \
--to=l.s.r@web.de \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).