Re: [PATCH 1/3] test-tool genzeros: initialize "zeros" to avoid SunCC warning

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2662 bytes --]

Hi Ævar,

On Thu, 13 Jan 2022, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Jan 12 2022, Taylor Blau wrote:
>
> > On Wed, Jan 12, 2022 at 03:21:46PM +0100, Johannes Schindelin wrote:
> >> > diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c
> >> > index 8ca988d6216..5dc89eda0cb 100644
> >> > --- a/t/helper/test-genzeros.c
> >> > +++ b/t/helper/test-genzeros.c
> >> > @@ -3,8 +3,7 @@
> >> >
> >> >  int cmd__genzeros(int argc, const char **argv)
> >> >  {
> >> > -	/* static, so that it is NUL-initialized */
> >> > -	static const char zeros[256 * 1024];
> >> > +	const char zeros[256 * 1024] = { 0 };
> >>
> >> This diff does two things: add an initializer, and turn the variable into
> >> a `static`. The former is the actual fix that is required. The latter is
> >> not. During the -rc phase, we do not want to see any of the latter. It is
> >> unnecessarily controversial and distracting, and can easily be postponed
> >> until January 25th, 2022.
> >
> > This assumes that making the declaration non-static isn't necessary to
> > fix the warning from SunCC.
>
> Just adding "= { 0 }" and retaining the "static" would FWIW make SunCC
> happy here.

Probably moving the `static` outside of the function would do the same,
and would be even more surgical a change.

> > I would guess that in reality it probably isn't, so removing the static
> > designation is a stray change, and this would have been easier to grok
> > as simply:
> >
> >     -	static const char zeros[256 * 1024];
> >     +	static const char zeros[256 * 1024] = { 0 };
> >
> > But to be honest I don't think it is _that_ big of a deal to make such a
> > small change during this point of the development cycle.
>
> We'd also need to change the comment, so:
>
> -	/* static, so that it is NUL-initialized */
> -	static const char zeros[256 * 1024];
> +	/* static, for no particular reason */
> +	static const char zeros[256 * 1024] = { 0 };
>
> Which is why I stripped the "static" off it, it was only there as a
> shorthand for doing the initialization, so once we're doing it ourselves
> it makes no sense to retain it for this invoked-only-once test helper.
>
> So I think this patch is good as-is.

That's stating the obvious.

> just adding the initializer would need even further explanation in the
> comment/commit message about the non-sensical end-state. I'm all for
> being careful in the rc period, but in this case I think we'd be
> overdoing it.

If this were the only instance where the patch is larger than strictly
necessary in the -rc phase, I would have let it slide, too.

Ciao,
Johannes