git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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é

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