Re: [PATCH -v2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
- From
Jeff King <peff@peff.net>
- Date
- Jun 23, 2017, 16:44 UTC
- Message-ID
- <20170623164403.bxilz7k5ny7hs466@sigill.intra.peff.net>
- In-Reply-To
- <20170623163606.27571-1-avarab@gmail.com>
On Fri, Jun 23, 2017 at 04:36:06PM +0000, Ævar Arnfjörð Bjarmason wrote:
> I believe this addresses the comments in the thread so far. Also Re: > René's "why const?" in a2673ce4-5cf8-6b40-d4db-8e2a49518138@web.de: > Because tzname_from_tz isn't changed in the body of the function, only > read.
Sure, it's not wrong. But that property is also held by 99% of the parameters that are passed by value. It's the normal style in our code base (and in most C code bases I know of) to never declare pass-by-value as const. It pollutes the interface and isn't something the caller cares about.
Without passing judgement on whether that style is good or not (though IMHO it is), making this one case different than all the others is a bad idea. It makes the reader wonder why it's different.
Show 10 quoted lines
> diff --git a/date.c b/date.c > index 1fd6d66375..17db07d905 100644 > --- a/date.c > +++ b/date.c > @@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode) > tm->tm_hour, tm->tm_min, tm->tm_sec, tz); > else if (mode->type == DATE_STRFTIME) > strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz, > - mode->local ? NULL : ""); > + mode->local);
You flipped the boolean here. That's OK by me. But in the definition...
> void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, > - int tz_offset, const char *tz_name) > + int tz_offset, const int tzname_from_tz)
Wouldn't tzname_from_tz only happen when we're _not_ in local mode? I suggested that name anticipating your second patch to actually compute it based on "tz". In local-mode it's not coming from tz, it's coming from secret unportable magic (the combination of localtime() and strftime()).
Show 10 quoted lines
> @@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
> fmt++;
> break;
> case 'Z':
> - if (tz_name) {
> - strbuf_addstr(&munged_fmt, tz_name);
> + if (!tzname_from_tz) {
> fmt++;
> break;
> }This logic matches your inversion in the caller, so it does the right thing. But I think the name is wrong, as above.
Show 18 quoted lines
> index 4559035c47..eba5d59a77 100644 > --- a/strbuf.h > +++ b/strbuf.h > @@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap); > > /** > * Add the time specified by `tm`, as formatted by `strftime`. > - * `tz_name` is used to expand %Z internally unless it's NULL. > * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west > * of Greenwich, and it's used to expand %z internally. However, tokens > * with modifiers (e.g. %Ez) are passed to `strftime`. > + * `tzname_from_tz` when set, means let `strftime` format %Z, instead > + * of intercepting it and doing our own formatting. > */ > extern void strbuf_addftime(struct strbuf *sb, const char *fmt, > const struct tm *tm, int tz_offset, > - const char *tz_name); > + const int omit_strftime_tz_name);
This would need the new name, too (whatever it is).
-Peff