[PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0

[PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0

[Ævar Arnfjörð Bjarmason]

This trivial set of patches fixes compiler complaints from SunCC on
Solaris that are new in v2.35.0-rc0.

The first two are only a minor annoyance, and not the first or only
warnings of those categories that we emit, but if we can avoid adding
new ones...

The third one is a generic integer overflow bug, and will probably
result in logic errors or failures in the reftable tests on some other
platform.

Ævar Arnfjörð Bjarmason (3):
  test-tool genzeros: initialize "zeros" to avoid SunCC warning
  reftable: remove unreachable "return" statements
  reftable tests: avoid "int" overflow, use "uint64_t"

 reftable/merged_test.c   | 4 ++--
 reftable/refname.c       | 1 -
 reftable/writer.c        | 1 -
 t/helper/test-genzeros.c | 3 +--
 4 files changed, 3 insertions(+), 6 deletions(-)

-- 
2.35.0.rc0.844.gb5945183dcf

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

[Ævar Arnfjörð Bjarmason]

It isn't important for optimization to have this be "static", so let's
just initialize it and avoid this warning on Sun Studio 12.5:

    "t/helper/test-genzeros.c", line 7: warning: const object should have initializer: zeros

This amends code added in df7000cd910 (test-tool genzeros: generate
large amounts of data more efficiently, 2021-11-02), and first tagged
with v2.35.0-rc0.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-genzeros.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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 };
 	intmax_t count;
 	ssize_t n;
 
-- 
2.35.0.rc0.844.gb5945183dcf

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

[Taylor Blau]

On Tue, Jan 11, 2022 at 05:40:21PM +0100, Ævar Arnfjörð Bjarmason wrote:
> It isn't important for optimization to have this be "static", so let's
> just initialize it and avoid this warning on Sun Studio 12.5:
>
>     "t/helper/test-genzeros.c", line 7: warning: const object should have initializer: zeros

I agree that whether or not this variable is declared statically does
not matter for our purposes, since we call cmd__genzeros() exactly once
per invocation of the test helper. So we're never paying the cost to
re-declare a variable on the stack since that stack frame only gets
created once.

And I don't care for the purposes of a reroll on such a trivial
question, but I do think that this patch leaves something out that was
included in the cover letter. Namely, that this is not the first such
warning of this kind from SunCC. Or in other words, that we have lots of
static const objects that we depend on being zero'd (and which we can
safely assume _are_ zerod, since they are declared statically), but that
each of them already generates a warning from SunCC.

Mentioning that would give readers an impression of why this was the
only spot touched instead of every static const variable.

But if you take that line of thinking further, I have a hard time
imagining that it's worth fixing small issues like this in piecemeal
when there are already many such warnings. We _could_ fix all of them at
once and amend Documentation/CodingGuidelines, but that seems like a lot
of work for an issue that does not seem to be causing much pain at all.

> 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 };

So this transformation is just fine, but I imagine that we probably
could have just as easily left it alone.

Thanks,
Taylor

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

[Johannes Schindelin]

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

Hi Ævar,

On Tue, 11 Jan 2022, Ævar Arnfjörð Bjarmason wrote:

> It isn't important for optimization to have this be "static", so let's
> just initialize it and avoid this warning on Sun Studio 12.5:
>
>     "t/helper/test-genzeros.c", line 7: warning: const object should have initializer: zeros
>
> This amends code added in df7000cd910 (test-tool genzeros: generate
> large amounts of data more efficiently, 2021-11-02), and first tagged
> with v2.35.0-rc0.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/helper/test-genzeros.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> 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.

Ciao,
Johannes

>  	intmax_t count;
>  	ssize_t n;
>
> --
> 2.35.0.rc0.844.gb5945183dcf
>
>

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

[Taylor Blau]

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.

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.

Thanks,
Taylor

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

[Ævar Arnfjörð Bjarmason]

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.

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

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

[Johannes Schindelin]

[-- 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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

It would make folks, who worry about having too large an item on the
stack to begin with, happy, too.  256kB on stack of a function that
does not make a call into a deep call chain would not matter all
that much, but it is a good principle to keep in mind.

We've worked around false "uninitialized" alarms from too picky
(versions of) compilers before by adding an otherwise unnecessary
initializers before, and I think this falls into the same category.

It is a separate matter if it is appropriate to worry about SunCC
this late in the cycle.  If this were "we were clean before, and
these small number of places add breakages", I would say yes.

But if this is "let's not add more of the same existing breakage
that we already have tons", we should not even be discussing about
such a change this late in the cycle (immediately after the new
offenders were added would have been more appropriate).

I offhand do not know which side of that line this one falls,
though.

Thanks.

[PATCH 2/3] reftable: remove unreachable "return" statements

[Ævar Arnfjörð Bjarmason]

Remove unreachable return statements added in acb533440fc (reftable:
implement refname validation, 2021-10-07) and f14bd719349 (reftable:
write reftable files, 2021-10-07).

This avoids the following warnings on SunCC 12.5 on
gcc211.fsffrance.org:

    "reftable/refname.c", line 135: warning: statement not reached
    "reftable/refname.c", line 135: warning: statement not reached

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reftable/refname.c | 1 -
 reftable/writer.c  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/reftable/refname.c b/reftable/refname.c
index 95734969324..136001bc2c7 100644
--- a/reftable/refname.c
+++ b/reftable/refname.c
@@ -132,7 +132,6 @@ static int validate_refname(const char *name)
 			return REFTABLE_REFNAME_ERROR;
 		name = next + 1;
 	}
-	return 0;
 }
 
 int validate_ref_record_addition(struct reftable_table tab,
diff --git a/reftable/writer.c b/reftable/writer.c
index 35c8649c9b7..70a7bf142a2 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -39,7 +39,6 @@ writer_reftable_block_stats(struct reftable_writer *w, uint8_t typ)
 		return &w->stats.log_stats;
 	}
 	abort();
-	return NULL;
 }
 
 /* write data, queuing the padding for the next write. Returns negative for
-- 
2.35.0.rc0.844.gb5945183dcf

Re: [PATCH 2/3] reftable: remove unreachable "return" statements

[Taylor Blau]

On Tue, Jan 11, 2022 at 05:40:22PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Remove unreachable return statements added in acb533440fc (reftable:
> implement refname validation, 2021-10-07) and f14bd719349 (reftable:
> write reftable files, 2021-10-07).
>
> This avoids the following warnings on SunCC 12.5 on
> gcc211.fsffrance.org:
>
>     "reftable/refname.c", line 135: warning: statement not reached
>     "reftable/refname.c", line 135: warning: statement not reached

Interesting. From a cursory reading, I agree with the assessment of
at least my compiler that these return statements are both unnecessary,
but...

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  reftable/refname.c | 1 -
>  reftable/writer.c  | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/reftable/refname.c b/reftable/refname.c
> index 95734969324..136001bc2c7 100644
> --- a/reftable/refname.c
> +++ b/reftable/refname.c
> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)
>  			return REFTABLE_REFNAME_ERROR;
>  		name = next + 1;
>  	}
> -	return 0;
>  }

In this case the loop inside of validate_refname() should always
terminate the function within the loop body. But removing this return
statement here relies on the compiler to determine that fact.

I could well imagine on the other end of the spectrum there exists a
compiler which _doesn't_ make this inference pass, and would complain
about the opposite thing as you're reporting from SunCC (i.e., that this
function which returns something other than void does not have a return
statement outside of the loop).

So in that sense, I disagree with the guidance of SunCC's warning. In
other words: by quelching this warning under one compiler, are we
introducing a new warning under a different/less advanced compiler?

>  int validate_ref_record_addition(struct reftable_table tab,
> diff --git a/reftable/writer.c b/reftable/writer.c
> index 35c8649c9b7..70a7bf142a2 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -39,7 +39,6 @@ writer_reftable_block_stats(struct reftable_writer *w, uint8_t typ)
>  		return &w->stats.log_stats;
>  	}
>  	abort();
> -	return NULL;
>  }

Here I'm less skeptical, since it's almost certain that any compiler
would recognize this call to abort() as terminating the whole program.
So it should be able to infer that anything after it is unreachable.

But even though I'm less skeptical, I'm not sure that I would make the
same bet (though in practice this one is probably fine since there are
likely plenty of functions which end in the more standard `die()` and do
not have a return path).

Can reftable call die()? Or is this the least-common denominator among
Git and libgit2 for terminating a running program?

Thanks,
Taylor

Re: [PATCH 2/3] reftable: remove unreachable "return" statements

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 11 2022, Taylor Blau wrote:

> On Tue, Jan 11, 2022 at 05:40:22PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Remove unreachable return statements added in acb533440fc (reftable:
>> implement refname validation, 2021-10-07) and f14bd719349 (reftable:
>> write reftable files, 2021-10-07).
>>
>> This avoids the following warnings on SunCC 12.5 on
>> gcc211.fsffrance.org:
>>
>>     "reftable/refname.c", line 135: warning: statement not reached
>>     "reftable/refname.c", line 135: warning: statement not reached
>
> Interesting. From a cursory reading, I agree with the assessment of
> at least my compiler that these return statements are both unnecessary,
> but...
>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  reftable/refname.c | 1 -
>>  reftable/writer.c  | 1 -
>>  2 files changed, 2 deletions(-)
>>
>> diff --git a/reftable/refname.c b/reftable/refname.c
>> index 95734969324..136001bc2c7 100644
>> --- a/reftable/refname.c
>> +++ b/reftable/refname.c
>> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)
>>  			return REFTABLE_REFNAME_ERROR;
>>  		name = next + 1;
>>  	}
>> -	return 0;
>>  }
>
> In this case the loop inside of validate_refname() should always
> terminate the function within the loop body. But removing this return
> statement here relies on the compiler to determine that fact.
>
> I could well imagine on the other end of the spectrum there exists a
> compiler which _doesn't_ make this inference pass, and would complain
> about the opposite thing as you're reporting from SunCC (i.e., that this
> function which returns something other than void does not have a return
> statement outside of the loop).
>
> So in that sense, I disagree with the guidance of SunCC's warning. In
> other words: by quelching this warning under one compiler, are we
> introducing a new warning under a different/less advanced compiler?

I'd think that any compiler who'd warn about this sort of thing at all
would be able to spot constructs like this one, which are basically:

    while (1) {
    	...
        if (x)
        	return;
	...
    }
    return; /* unreachable */

Where the elided code contains no "break", "goto" or other mechanism for
exiting the for-loop.

I.e. GCC and Clang don't bother to note the unreachable code, but I
don't think the reverse will be true, that a compiler will say that a
"return" is missing there. Having a function be just a loop body that
returns an some point is a common pattern.

>>  int validate_ref_record_addition(struct reftable_table tab,
>> diff --git a/reftable/writer.c b/reftable/writer.c
>> index 35c8649c9b7..70a7bf142a2 100644
>> --- a/reftable/writer.c
>> +++ b/reftable/writer.c
>> @@ -39,7 +39,6 @@ writer_reftable_block_stats(struct reftable_writer *w, uint8_t typ)
>>  		return &w->stats.log_stats;
>>  	}
>>  	abort();
>> -	return NULL;
>>  }
>
> Here I'm less skeptical, since it's almost certain that any compiler
> would recognize this call to abort() as terminating the whole program.
> So it should be able to infer that anything after it is unreachable.

That's also correct, but in terms of compiler implementations I'd think
you'd get basic loop flow analysis first, and the annotation of
unreturn-able functions like abort() or a custom die() later.
> ...

Re: [PATCH 2/3] reftable: remove unreachable "return" statements

[Taylor Blau]

On Wed, Jan 12, 2022 at 01:47:40PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  reftable/refname.c | 1 -
> >>  reftable/writer.c  | 1 -
> >>  2 files changed, 2 deletions(-)
> >>
> >> diff --git a/reftable/refname.c b/reftable/refname.c
> >> index 95734969324..136001bc2c7 100644
> >> --- a/reftable/refname.c
> >> +++ b/reftable/refname.c
> >> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)
> >>  			return REFTABLE_REFNAME_ERROR;
> >>  		name = next + 1;
> >>  	}
> >> -	return 0;
> >>  }
> >
> > In this case the loop inside of validate_refname() should always
> > terminate the function within the loop body. But removing this return
> > statement here relies on the compiler to determine that fact.
> >
> > I could well imagine on the other end of the spectrum there exists a
> > compiler which _doesn't_ make this inference pass, and would complain
> > about the opposite thing as you're reporting from SunCC (i.e., that this
> > function which returns something other than void does not have a return
> > statement outside of the loop).
> >
> > So in that sense, I disagree with the guidance of SunCC's warning. In
> > other words: by quelching this warning under one compiler, are we
> > introducing a new warning under a different/less advanced compiler?
>
> I'd think that any compiler who'd warn about this sort of thing at all
> would be able to spot constructs like this one, which are basically:
>
>     while (1) {
>     	...
>         if (x)
>         	return;
> 	...
>     }
>     return; /* unreachable */
>
> Where the elided code contains no "break", "goto" or other mechanism for
> exiting the for-loop.
>
> I.e. GCC and Clang don't bother to note the unreachable code, but I
> don't think the reverse will be true, that a compiler will say that a
> "return" is missing there. Having a function be just a loop body that
> returns an some point is a common pattern.

Right, but I'm more concerned about a less advanced compiler that would
complain about the absence of a `return` statement as the last
instruction in a non-void function.

This is probably all academic, anyway, since less advanced compilers
probably have other issues compiling Git as it stands today, but
fundamentally I think that SunCC's warnings here are at the very least
inconsiderate of less advanced compilers.

To me, the safest thing to do would be to leave the code as-is and drop
this patch.

Thanks,
Taylor

Re: [PATCH 2/3] reftable: remove unreachable "return" statements

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 12 2022, Taylor Blau wrote:

> On Wed, Jan 12, 2022 at 01:47:40PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >> ---
>> >>  reftable/refname.c | 1 -
>> >>  reftable/writer.c  | 1 -
>> >>  2 files changed, 2 deletions(-)
>> >>
>> >> diff --git a/reftable/refname.c b/reftable/refname.c
>> >> index 95734969324..136001bc2c7 100644
>> >> --- a/reftable/refname.c
>> >> +++ b/reftable/refname.c
>> >> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)
>> >>  			return REFTABLE_REFNAME_ERROR;
>> >>  		name = next + 1;
>> >>  	}
>> >> -	return 0;
>> >>  }
>> >
>> > In this case the loop inside of validate_refname() should always
>> > terminate the function within the loop body. But removing this return
>> > statement here relies on the compiler to determine that fact.
>> >
>> > I could well imagine on the other end of the spectrum there exists a
>> > compiler which _doesn't_ make this inference pass, and would complain
>> > about the opposite thing as you're reporting from SunCC (i.e., that this
>> > function which returns something other than void does not have a return
>> > statement outside of the loop).
>> >
>> > So in that sense, I disagree with the guidance of SunCC's warning. In
>> > other words: by quelching this warning under one compiler, are we
>> > introducing a new warning under a different/less advanced compiler?
>>
>> I'd think that any compiler who'd warn about this sort of thing at all
>> would be able to spot constructs like this one, which are basically:
>>
>>     while (1) {
>>     	...
>>         if (x)
>>         	return;
>> 	...
>>     }
>>     return; /* unreachable */
>>
>> Where the elided code contains no "break", "goto" or other mechanism for
>> exiting the for-loop.
>>
>> I.e. GCC and Clang don't bother to note the unreachable code, but I
>> don't think the reverse will be true, that a compiler will say that a
>> "return" is missing there. Having a function be just a loop body that
>> returns an some point is a common pattern.
>
> Right, but I'm more concerned about a less advanced compiler that would
> complain about the absence of a `return` statement as the last
> instruction in a non-void function.
>
> This is probably all academic, anyway, since less advanced compilers
> probably have other issues compiling Git as it stands today, but
> fundamentally I think that SunCC's warnings here are at the very least
> inconsiderate of less advanced compilers.
>
> To me, the safest thing to do would be to leave the code as-is and drop
> this patch.

I really don't see that, sorry. We have an actual example of a compliler
that does emit a warning new in this rc on this code, but AFAICT your
concern is purely hypothetical.

Such a hypothetical compiler would already be emitting a firehose of
false-positive warnings in our or any non-trivial C codebase,
e.g. builtin/bisect--helper.c:bisect_run(), show-branch.c:version_cmp()
and fsck.c:count_leading_dotdots() would all warn (and I just picked
three examples from a quick grep, there's a lot more of them).

So I don't think we need to be concerned about such a hypothetical
compiler. I think anyone doing such flow analysis tries to do it well
enough to not make the warning entirely useless.

Aside: SunCC does get it wrong in some cases, but it's more obscure
code, mainly from jumping into a for-loop via "goto", and not
propagating understanding the implications of NORETURN in some cases (or
maybe we're just using the GCC-specific one in that case).

Re: [PATCH 2/3] reftable: remove unreachable "return" statements

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3165 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 01:47:40PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> >> ---
> >> >>  reftable/refname.c | 1 -
> >> >>  reftable/writer.c  | 1 -
> >> >>  2 files changed, 2 deletions(-)
> >> >>
> >> >> diff --git a/reftable/refname.c b/reftable/refname.c
> >> >> index 95734969324..136001bc2c7 100644
> >> >> --- a/reftable/refname.c
> >> >> +++ b/reftable/refname.c
> >> >> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)
> >> >>  			return REFTABLE_REFNAME_ERROR;
> >> >>  		name = next + 1;
> >> >>  	}
> >> >> -	return 0;
> >> >>  }
> >> >
> >> > In this case the loop inside of validate_refname() should always
> >> > terminate the function within the loop body. But removing this return
> >> > statement here relies on the compiler to determine that fact.
> >> >
> >> > I could well imagine on the other end of the spectrum there exists a
> >> > compiler which _doesn't_ make this inference pass, and would complain
> >> > about the opposite thing as you're reporting from SunCC (i.e., that this
> >> > function which returns something other than void does not have a return
> >> > statement outside of the loop).
> >> >
> >> > So in that sense, I disagree with the guidance of SunCC's warning. In
> >> > other words: by quelching this warning under one compiler, are we
> >> > introducing a new warning under a different/less advanced compiler?
> >>
> >> I'd think that any compiler who'd warn about this sort of thing at all
> >> would be able to spot constructs like this one, which are basically:
> >>
> >>     while (1) {
> >>     	...
> >>         if (x)
> >>         	return;
> >> 	...
> >>     }
> >>     return; /* unreachable */
> >>
> >> Where the elided code contains no "break", "goto" or other mechanism for
> >> exiting the for-loop.
> >>
> >> I.e. GCC and Clang don't bother to note the unreachable code, but I
> >> don't think the reverse will be true, that a compiler will say that a
> >> "return" is missing there. Having a function be just a loop body that
> >> returns an some point is a common pattern.
> >
> > Right, but I'm more concerned about a less advanced compiler that would
> > complain about the absence of a `return` statement as the last
> > instruction in a non-void function.
> >
> > This is probably all academic, anyway, since less advanced compilers
> > probably have other issues compiling Git as it stands today, but
> > fundamentally I think that SunCC's warnings here are at the very least
> > inconsiderate of less advanced compilers.
> >
> > To me, the safest thing to do would be to leave the code as-is and drop
> > this patch.
>
> I really don't see that, sorry. We have an actual example of a compliler
> that does emit a warning new in this rc on this code, but AFAICT your
> concern is purely hypothetical.

It's just a warning.

So I concur with Taylor. This patch does not need to go into v2.35.0.

Ciao,
Johannes

Re: [PATCH 2/3] reftable: remove unreachable "return" statements

[Johannes Sixt]

Am 12.01.22 um 13:47 schrieb Ævar Arnfjörð Bjarmason:
> 
> On Tue, Jan 11 2022, Taylor Blau wrote:
> 
>> On Tue, Jan 11, 2022 at 05:40:22PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>> Remove unreachable return statements added in acb533440fc (reftable:
>>> implement refname validation, 2021-10-07) and f14bd719349 (reftable:
>>> write reftable files, 2021-10-07).
>>>
>>> This avoids the following warnings on SunCC 12.5 on
>>> gcc211.fsffrance.org:
>>>
>>>     "reftable/refname.c", line 135: warning: statement not reached
>>>     "reftable/refname.c", line 135: warning: statement not reached
>>
>> Interesting. From a cursory reading, I agree with the assessment of
>> at least my compiler that these return statements are both unnecessary,
>> but...
>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>>  reftable/refname.c | 1 -
>>>  reftable/writer.c  | 1 -
>>>  2 files changed, 2 deletions(-)
>>>
>>> diff --git a/reftable/refname.c b/reftable/refname.c
>>> index 95734969324..136001bc2c7 100644
>>> --- a/reftable/refname.c
>>> +++ b/reftable/refname.c
>>> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)
>>>  			return REFTABLE_REFNAME_ERROR;
>>>  		name = next + 1;
>>>  	}
>>> -	return 0;
>>>  }
>>
>> In this case the loop inside of validate_refname() should always
>> terminate the function within the loop body. But removing this return
>> statement here relies on the compiler to determine that fact.
>>
>> I could well imagine on the other end of the spectrum there exists a
>> compiler which _doesn't_ make this inference pass, and would complain
>> about the opposite thing as you're reporting from SunCC (i.e., that this
>> function which returns something other than void does not have a return
>> statement outside of the loop).
>>
>> So in that sense, I disagree with the guidance of SunCC's warning. In
>> other words: by quelching this warning under one compiler, are we
>> introducing a new warning under a different/less advanced compiler?
> 
> I'd think that any compiler who'd warn about this sort of thing at all
> would be able to spot constructs like this one, which are basically:
> 
>     while (1) {
>     	...
>         if (x)
>         	return;
> 	...
>     }
>     return; /* unreachable */
> 
> Where the elided code contains no "break", "goto" or other mechanism for
> exiting the for-loop.

Why not just sidestep the problematic case:

diff --git a/reftable/refname.c b/reftable/refname.c
index 9573496932..4f89956187 100644
--- a/reftable/refname.c
+++ b/reftable/refname.c
@@ -120,17 +120,17 @@ static int modification_has_ref_with_prefix(struct modification *mod,
 static int validate_refname(const char *name)
 {
 	while (1) {
 		char *next = strchr(name, '/');
 		if (!*name) {
 			return REFTABLE_REFNAME_ERROR;
 		}
 		if (!next) {
-			return 0;
+			break;
 		}
 		if (next - name == 0 || (next - name == 1 && *name == '.') ||
 		    (next - name == 2 && name[0] == '.' && name[1] == '.'))
 			return REFTABLE_REFNAME_ERROR;
 		name = next + 1;
 	}
 	return 0;
 }

Sure, there are returns in the loop, but they are clearly error cases.
The regular exit is now at the end of the function.

-- Hannes

Re: [PATCH 2/3] reftable: remove unreachable "return" statements

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Why not just sidestep the problematic case:
>
> diff --git a/reftable/refname.c b/reftable/refname.c
> index 9573496932..4f89956187 100644
> --- a/reftable/refname.c
> +++ b/reftable/refname.c
> @@ -120,17 +120,17 @@ static int modification_has_ref_with_prefix(struct modification *mod,
>  static int validate_refname(const char *name)
>  {
>  	while (1) {
>  		char *next = strchr(name, '/');
>  		if (!*name) {
>  			return REFTABLE_REFNAME_ERROR;
>  		}
>  		if (!next) {
> -			return 0;
> +			break;
>  		}
>  		if (next - name == 0 || (next - name == 1 && *name == '.') ||
>  		    (next - name == 2 && name[0] == '.' && name[1] == '.'))
>  			return REFTABLE_REFNAME_ERROR;
>  		name = next + 1;
>  	}
>  	return 0;
>  }
>
> Sure, there are returns in the loop, but they are clearly error cases.
> The regular exit is now at the end of the function.

;-)

[PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Ævar Arnfjörð Bjarmason]

Change code added in 1ae2b8cda84 (reftable: add merged table view,
2021-10-07) to consistently use the "uint64_t" type. These "min" and
"max" variables get passed in the body of this function to a function
whose prototype is:

    [...] reftable_writer_set_limits([...], uint64_t min, uint64_t max

This avoids the following warning on SunCC 12.5 on
gcc211.fsffrance.org:

    "reftable/merged_test.c", line 27: warning: initializer does not fit or is out of range: 0xffffffff

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reftable/merged_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index 24461e8a802..b87ff495dfd 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -24,8 +24,8 @@ license that can be found in the LICENSE file or at
 static void write_test_table(struct strbuf *buf,
 			     struct reftable_ref_record refs[], int n)
 {
-	int min = 0xffffffff;
-	int max = 0;
+	uint64_t min = 0xffffffff;
+	uint64_t max = 0;
 	int i = 0;
 	int err;
 
-- 
2.35.0.rc0.844.gb5945183dcf

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Taylor Blau]

On Tue, Jan 11, 2022 at 05:40:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/reftable/merged_test.c b/reftable/merged_test.c
> index 24461e8a802..b87ff495dfd 100644
> --- a/reftable/merged_test.c
> +++ b/reftable/merged_test.c
> @@ -24,8 +24,8 @@ license that can be found in the LICENSE file or at
>  static void write_test_table(struct strbuf *buf,
>  			     struct reftable_ref_record refs[], int n)
>  {
> -	int min = 0xffffffff;
> -	int max = 0;
> +	uint64_t min = 0xffffffff;
> +	uint64_t max = 0;

Han-Wen: it looks like the loop below the context here is to set the
min/max of update_index over all of the ref records?

If so, making these comparisons all unsigned makes sense to me. In
practice it's probably fine at least from a signedness perspective,
since the compiler _should_ be coercing both operands to unsigned.

But perhaps not so from a width perspective, if sizeof(int) != 8 (though
I suspect in practice that we are unlikely to have enough possible
values of update_index for that to matter).

In any case, you're only setting the lower half of `min` high. Maybe:

    uint64_t min = ~0ul;

instead?

Thanks,
Taylor

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Han-Wen Nienhuys]

On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Jan 11, 2022 at 05:40:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > diff --git a/reftable/merged_test.c b/reftable/merged_test.c
> > index 24461e8a802..b87ff495dfd 100644
> > --- a/reftable/merged_test.c
> > +++ b/reftable/merged_test.c
> > @@ -24,8 +24,8 @@ license that can be found in the LICENSE file or at
> >  static void write_test_table(struct strbuf *buf,
> >                            struct reftable_ref_record refs[], int n)
> >  {
> > -     int min = 0xffffffff;
> > -     int max = 0;
> > +     uint64_t min = 0xffffffff;
> > +     uint64_t max = 0;
>
> Han-Wen: it looks like the loop below the context here is to set the
> min/max of update_index over all of the ref records?

correct.

> But perhaps not so from a width perspective, if sizeof(int) != 8 (though
> I suspect in practice that we are unlikely to have enough possible
> values of update_index for that to matter).

correct.

> In any case, you're only setting the lower half of `min` high. Maybe:
>
>     uint64_t min = ~0ul;

yeah, that works.


-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Taylor Blau]

On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
> > In any case, you're only setting the lower half of `min` high. Maybe:
> >
> >     uint64_t min = ~0ul;
>
> yeah, that works.

I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
somebody more confident than I in this area would be welcome :).

Thanks,
Taylor

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Johannes Sixt]

Am 11.01.22 um 20:41 schrieb Taylor Blau:
> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
>>> In any case, you're only setting the lower half of `min` high. Maybe:
>>>
>>>     uint64_t min = ~0ul;
>>
>> yeah, that works.
> 
> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
> somebody more confident than I in this area would be welcome :).

It does not work on Windows: unsigned long is 32 bits wide. You have to
make it

   uint64_t min = ~(uint64_t)0;

-- Hannes

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Taylor Blau]

On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
> Am 11.01.22 um 20:41 schrieb Taylor Blau:
> > On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
> >> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
> >>> In any case, you're only setting the lower half of `min` high. Maybe:
> >>>
> >>>     uint64_t min = ~0ul;
> >>
> >> yeah, that works.
> >
> > I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
> > somebody more confident than I in this area would be welcome :).
>
> It does not work on Windows: unsigned long is 32 bits wide. You have to
> make it
>
>    uint64_t min = ~(uint64_t)0;

Perfect; this is exactly what I was looking for. Thanks!

Taylor

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Johannes Sixt]

Am 11.01.22 um 21:18 schrieb Taylor Blau:
> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
>> Am 11.01.22 um 20:41 schrieb Taylor Blau:
>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
>>>>> In any case, you're only setting the lower half of `min` high. Maybe:
>>>>>
>>>>>     uint64_t min = ~0ul;
>>>>
>>>> yeah, that works.
>>>
>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
>>> somebody more confident than I in this area would be welcome :).
>>
>> It does not work on Windows: unsigned long is 32 bits wide. You have to
>> make it
>>
>>    uint64_t min = ~(uint64_t)0;
> 
> Perfect; this is exactly what I was looking for. Thanks!

Actually, on second thought, UINT64_MAX would be even better.

-- Hannes

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Taylor Blau]

On Tue, Jan 11, 2022 at 09:21:11PM +0100, Johannes Sixt wrote:
> Am 11.01.22 um 21:18 schrieb Taylor Blau:
> > On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
> >> Am 11.01.22 um 20:41 schrieb Taylor Blau:
> >>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
> >>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
> >>>>> In any case, you're only setting the lower half of `min` high. Maybe:
> >>>>>
> >>>>>     uint64_t min = ~0ul;
> >>>>
> >>>> yeah, that works.
> >>>
> >>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
> >>> somebody more confident than I in this area would be welcome :).
> >>
> >> It does not work on Windows: unsigned long is 32 bits wide. You have to
> >> make it
> >>
> >>    uint64_t min = ~(uint64_t)0;
> >
> > Perfect; this is exactly what I was looking for. Thanks!
>
> Actually, on second thought, UINT64_MAX would be even better.

:-). I think that either is probably fine; I couldn't remember if
UINT64_MAX was part of POSIX or not (and clearly didn't bother to check!)

Thanks,
Taylor

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Johannes Schindelin]

Hi Taylor,

On Tue, 11 Jan 2022, Taylor Blau wrote:

> On Tue, Jan 11, 2022 at 09:21:11PM +0100, Johannes Sixt wrote:
> > Am 11.01.22 um 21:18 schrieb Taylor Blau:
> > > On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
> > >> Am 11.01.22 um 20:41 schrieb Taylor Blau:
> > >>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
> > >>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
> > >>>>> In any case, you're only setting the lower half of `min` high. Maybe:
> > >>>>>
> > >>>>>     uint64_t min = ~0ul;
> > >>>>
> > >>>> yeah, that works.
> > >>>
> > >>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
> > >>> somebody more confident than I in this area would be welcome :).
> > >>
> > >> It does not work on Windows: unsigned long is 32 bits wide. You have to
> > >> make it
> > >>
> > >>    uint64_t min = ~(uint64_t)0;
> > >
> > > Perfect; this is exactly what I was looking for. Thanks!
> >
> > Actually, on second thought, UINT64_MAX would be even better.
>
> :-). I think that either is probably fine; I couldn't remember if
> UINT64_MAX was part of POSIX or not (and clearly didn't bother to check!)

The best solution, of course, would be to `git grep` through the code and
see that UINT64_MAX is not used at all.

And that brings us to the question whether we really need to ensure that
exactly, precisely 64 bit are used for this variable? The answer is: no.
We may need it to be larger than 32-bit, so why not go for `uintmax_t` and
`UINTMAX_MAX`, both of which _are_ already used in Git's source code?

Ciao,
Dscho

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 11.01.22 um 21:18 schrieb Taylor Blau:
>> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
>>> Am 11.01.22 um 20:41 schrieb Taylor Blau:
>>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
>>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
>>>>>> In any case, you're only setting the lower half of `min` high. Maybe:
>>>>>>
>>>>>>     uint64_t min = ~0ul;
>>>>>
>>>>> yeah, that works.
>>>>
>>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
>>>> somebody more confident than I in this area would be welcome :).
>>>
>>> It does not work on Windows: unsigned long is 32 bits wide. You have to
>>> make it
>>>
>>>    uint64_t min = ~(uint64_t)0;
>> 
>> Perfect; this is exactly what I was looking for. Thanks!

That sounds perfect.

> Actually, on second thought, UINT64_MAX would be even better.

I wouldn't introduce use of UINT64_MAX, which "git grep" does not
produce any hits for.

Unless it is very early in a development cycle, that is, in which
case we have enough time to help platforms that are not quite POSIX.

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Taylor Blau]

On Wed, Jan 12, 2022 at 11:02:05AM -0800, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Am 11.01.22 um 21:18 schrieb Taylor Blau:
> >> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
> >>> Am 11.01.22 um 20:41 schrieb Taylor Blau:
> >>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
> >>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
> >>>>>> In any case, you're only setting the lower half of `min` high. Maybe:
> >>>>>>
> >>>>>>     uint64_t min = ~0ul;
> >>>>>
> >>>>> yeah, that works.
> >>>>
> >>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
> >>>> somebody more confident than I in this area would be welcome :).
> >>>
> >>> It does not work on Windows: unsigned long is 32 bits wide. You have to
> >>> make it
> >>>
> >>>    uint64_t min = ~(uint64_t)0;
> >>
> >> Perfect; this is exactly what I was looking for. Thanks!
>
> That sounds perfect.
>
> > Actually, on second thought, UINT64_MAX would be even better.
>
> I wouldn't introduce use of UINT64_MAX, which "git grep" does not
> produce any hits for.

> Unless it is very early in a development cycle, that is, in which
> case we have enough time to help platforms that are not quite POSIX.

Yep, I agree that avoiding introducing the first instance of UINT64_MAX
in our tree is worth doing (probably in general, but certainly now that
we're past even -rc0).

Either `~(uint64_t)0` or `UINTMAX_MAX` would be fine with me.

Thanks,
Taylor

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 12 2022, Taylor Blau wrote:

> On Wed, Jan 12, 2022 at 11:02:05AM -0800, Junio C Hamano wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>> > Am 11.01.22 um 21:18 schrieb Taylor Blau:
>> >> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
>> >>> Am 11.01.22 um 20:41 schrieb Taylor Blau:
>> >>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
>> >>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
>> >>>>>> In any case, you're only setting the lower half of `min` high. Maybe:
>> >>>>>>
>> >>>>>>     uint64_t min = ~0ul;
>> >>>>>
>> >>>>> yeah, that works.
>> >>>>
>> >>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
>> >>>> somebody more confident than I in this area would be welcome :).
>> >>>
>> >>> It does not work on Windows: unsigned long is 32 bits wide. You have to
>> >>> make it
>> >>>
>> >>>    uint64_t min = ~(uint64_t)0;
>> >>
>> >> Perfect; this is exactly what I was looking for. Thanks!
>>
>> That sounds perfect.
>>
>> > Actually, on second thought, UINT64_MAX would be even better.
>>
>> I wouldn't introduce use of UINT64_MAX, which "git grep" does not
>> produce any hits for.
>
>> Unless it is very early in a development cycle, that is, in which
>> case we have enough time to help platforms that are not quite POSIX.
>
> Yep, I agree that avoiding introducing the first instance of UINT64_MAX
> in our tree is worth doing (probably in general, but certainly now that
> we're past even -rc0).
>
> Either `~(uint64_t)0` or `UINTMAX_MAX` would be fine with me.

The reason I left it at 0xffffffff is because the current test clearly
doesn't care about using the maximum width of the type, and I was just
trying to get rid of the associated compiler warning.

So I'll leave it to Han-Wen to comment on if the "max" being the maximum
of the type is actually important here.

As far as what we'd pick to get the maximum type value goes, we should
just prefer whatever we use for that already in that codebase, and we've
got this in a related file there:
    
    reftable/generic.c:     struct reftable_log_record log = {
    reftable/generic.c-             .refname = (char *)name,
    reftable/generic.c-             .update_index = ~((uint64_t)0),
    reftable/generic.c-     };

(Which is what Johannes Sixt independently suggested upthread in
<45baffd7-c9f3-cc52-47b4-ea0fee0182a8@kdbg.org>).

Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> The reason I left it at 0xffffffff is because the current test clearly
> doesn't care about using the maximum width of the type, and I was just
> trying to get rid of the associated compiler warning.

Sounds sane.  Let's take the original one you sent out, and those
who want to make things consistent can swap it with ~((uint64_t)0)
after the release.

Thanks.

Re: [PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0

[Han-Wen Nienhuys]

On Tue, Jan 11, 2022 at 5:40 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> This trivial set of patches fixes compiler complaints from SunCC on
> Solaris that are new in v2.35.0-rc0.

Thanks, looks fine in principle.

For the switch, maybe the block types should be an enum. Is there
tooling that would detect a missing case in that case, so we could
drop the abort() ?

For the 0xffffffff constant, there is probably a better symbolic
constant somewhere. Or could we say (1<<64) -1 without causing
overflow?


-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0

[René Scharfe]

Am 11.01.22 um 18:06 schrieb Han-Wen Nienhuys:
> On Tue, Jan 11, 2022 at 5:40 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> This trivial set of patches fixes compiler complaints from SunCC on
>> Solaris that are new in v2.35.0-rc0.
>
> Thanks, looks fine in principle.
>
> For the switch, maybe the block types should be an enum. Is there
> tooling that would detect a missing case in that case, so we could
> drop the abort() ?
>
> For the 0xffffffff constant, there is probably a better symbolic
> constant somewhere. Or could we say (1<<64) -1 without causing
> overflow?

How about -1?

Or you could initialize min and max to the actual value of the first
item instead of to the absolute limits of the data type.

René

Re: [PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0

[Emily Shaffer]

On Tue, Jan 11, 2022 at 5:21 PM Emily Shaffer <emilyshaffer@google.com> wrote:

ugh, sorry for the thumbfinger. Please ignore.