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.
> 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.
> @@ -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. :)
> @@ -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.
> @@ -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
