On Thu, Oct 28, 2021 at 2:46 PM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
>
> On Thu, 28 Oct 2021, Carlo Marcelo Arenas Belón wrote:
>
> > Allow tests that assume a 64-bit size_t to be skipped in 32-bit platforms
> > and regardless of the size of long.
>
> Makes sense, but...
>
> >
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx>
> > ---
> > t/test-lib.sh | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index fc1e521519..5fa7fb5719 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -1687,6 +1687,10 @@ build_option () {
> > sed -ne "s/^$1: //p"
> > }
> >
> > +test_lazy_prereq IS_64BIT '
>
> This should be `SIZE_T_IS_64BIT`.
Fair point, but...
> > + test 8 -eq "$(build_option sizeof-size_t)"
>
> Since this is clearly copied from `LONG_IS_64BIT`, why the change from
> `-le` to `-eq`? It is at least inconsistent to use anything different
> here.
My assumption is that the check for sizeof(size_t) we have is really
about finding the bit width of the platform, and we currently support
2 of them (32-bit and 64-bit), which is why the name I chose was
"IS_64BIT" and also why I was strict on it being exactly 8 bytes
(considering all platforms git supports have bytes with 8 bits).
It can go eitherway IMHO, and your point about being inconsistent
(with my lack of explanation in the commit) suggests we should instead
use your proposal, do you want me to resend or could adjust them in
your tree?
Carlo
PS. I think we should also add a "TODO" comment in the code, but other
than that could also take my "Reviewed-by" for the series
