| Message ID | 20211028205649.84036-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow clean/smudge filters to handle huge files in the LLP64 data model | expand |
Hi Carlo, 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@gmail.com> > --- > 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`. > + 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. Ciao, Dscho > +' > + > test_lazy_prereq LONG_IS_64BIT ' > test 8 -le "$(build_option sizeof-long)" > ' > -- > 2.33.0.1155.gbdb71ac078 > > >
On Thu, Oct 28, 2021 at 2:46 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> 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@gmail.com> > > --- > > 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
Carlo Arenas <carenas@gmail.com> writes: >> 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? Is LONG_IS_64BIT used to ensure that long is _at least_ 64 bit? If so, perhaps its name may need to be rethought. On the other hand, if it is meant to ensure that long is exactly 64 bit, then it should use -eq to compare. And from that point of view, SIZE_T_IS_64BIT and use of -eq look consistent with each other, I would think. Thanks.
Hi Junio, On Thu, 28 Oct 2021, Junio C Hamano wrote: > Carlo Arenas <carenas@gmail.com> writes: > > >> 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? > > Is LONG_IS_64BIT used to ensure that long is _at least_ 64 bit? If > so, perhaps its name may need to be rethought. On the other hand, > if it is meant to ensure that long is exactly 64 bit, then it should > use -eq to compare. `LONG_IS_64BIT` is used by the `tar` tests to ensure that `long` can represent file sizes larger than 4GB. So yeah, it is an `_AT_LEAST_` instead of an `_IS_`. Not -rc material, though. Ciao, Dscho
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 ' + test 8 -eq "$(build_option sizeof-size_t)" +' + test_lazy_prereq LONG_IS_64BIT ' test 8 -le "$(build_option sizeof-long)" '
Allow tests that assume a 64-bit size_t to be skipped in 32-bit platforms and regardless of the size of long. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- t/test-lib.sh | 4 ++++ 1 file changed, 4 insertions(+)