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

From: Junio C Hamano <gitster@pobox.com>
Date: May 30, 2024, 16:07 UTC
Message-ID: <xmqq7cfbqo5m.fsf@gitster.g>
In-Reply-To: <ZlhjVIEld7_T0q-M@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 30 quoted lines

> On Wed, May 29, 2024 at 10:43:47AM -0700, Junio C Hamano wrote:
>> 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.

Oh, I do *not* want to see.

	char a_string[] = "a";
	char *names[] = { a_string, ... };

As a way to workaround the -Wwritable-strings warnings, what we see in this patch is much better.

I was referring to a redoing of this series in the other direction. The earlier one that introduced

	char refspec_string[] = "refs/heads/*";

and rewrite assignments of

	non_const_pointer_to_char = "refs/heads/*";

with

	non_const_pointer_to_char = refspec_string;

would have been better if it were done without refspec_string[] array. It is a runtime bug to overwrite the refspec_string[] via the non_const_pointer_to_char, but use of refspec_string[] would make it impossible for us to catch.