Re: [PATCH 05/19] reftable: improve const correctness when assigning string constants

From: Patrick Steinhardt <ps@pks.im>
Date: May 30, 2024, 11:30 UTC
Message-ID: <ZlhjVIEld7_T0q-M@tanuki>
In-Reply-To: <xmqqed9ky0mk.fsf@gitster.g>

On Wed, May 29, 2024 at 10:43:47AM -0700, Junio C Hamano wrote:

Show 23 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/reftable/basics_test.c b/reftable/basics_test.c
> > index 997c4d9e01..af9209d535 100644
> > --- a/reftable/basics_test.c
> > +++ b/reftable/basics_test.c
> > @@ -58,8 +58,8 @@ static void test_binsearch(void)
> >  
> >  static void test_names_length(void)
> >  {
> > -	char *a[] = { "a", "b", NULL };
> > -	EXPECT(names_length(a) == 2);
> > +	char *names[] = { (char *)"a", (char *)"b", NULL };
> > +	EXPECT(names_length(names) == 2);
> >  }
> 
> I would have preferred to see this kind of rewrite more than
> separate and clearly writable variables that are initialied with the
> constant contents e.g. branches[] = "refs/heads/*", we saw in
> earlier steps.  Wouldn't that approach, combined with making the
> literal constants stored in read-only segment to trigger runtime
> failure when a bug causes the "unfortunately non-const" variables
> to be written, give us a better result?

Depends on what we mean by "better", I guess. But yeah, I was torn myself when writing this commit because there are so many string constants in the reftable tests that we assign to non-constant fields. I didn't find the result particularly easy to read when putting each of the constants into a separate variable.

Revisiting this again though I don't think it's all that bad. I'll adapt accordingly.

Patrick