Re: [RFC PATCH 4/4] tools/nolibc: add tests for the integer limits in stdint.h

[Vincent Dagonneau]

Hi Thomas,

On Sun, Feb 19, 2023, at 14:15, Willy Tarreau wrote:
> Hi Thomas,
>
> On Sun, Feb 19, 2023 at 07:04:04PM +0000, Thomas Weißschuh wrote:
>> > +#elif __SIZEOF_LONG__ == 4
>> > +		CASE_TEST(limit_intptr_min);        EXPECT_EQ(1, INTPTR_MIN,  (intptr_t)  0x80000000); break;
>> > +		CASE_TEST(limit_intptr_max);        EXPECT_EQ(1, INTPTR_MAX,  (intptr_t)  0x7fffffff); break;
>> > +		CASE_TEST(limit_uintptr_max);       EXPECT_EQ(1, UINTPTR_MAX, (uintptr_t) 0xffffffffU); break;
>> > +		CASE_TEST(limit_ptrdiff_min);       EXPECT_EQ(1, PTRDIFF_MIN, (ptrdiff_t) 0x80000000); break;
>> > +		CASE_TEST(limit_ptrdiff_max);       EXPECT_EQ(1, PTRDIFF_MAX, (ptrdiff_t) 0x7fffffff); break;
>> > +		CASE_TEST(limit_ptrdiff_min);       EXPECT_EQ(1, PTRDIFF_MIN, (ptrdiff_t) 0x80000000); break;
>> > +		CASE_TEST(limit_ptrdiff_max);       EXPECT_EQ(1, PTRDIFF_MAX, (ptrdiff_t) 0x7fffffff); break;
>> 
>> ptrdiff tests are duplicate.
>
> Argh, I thought I had already removed these duplicates, I noticed them
> previously indeed. Vincent, please address this in your next iteration.
>

Oops, my mistake, sorry about that. I removed it from v5. 

>> > +		CASE_TEST(limit_size_max);          EXPECT_EQ(1, SIZE_MAX,    (size_t)    0xffffffffU); break;
>> > +#else
>> > +# warning "__SIZEOF_LONG__ is undefined"
>> 
>> Why not #error?
>
> It's just a matter of choice. Since the tool's goal is to spot errors,
> and if possible several at once, I find it preferable to still not fail
> on other tests, as often when you get multiple failures it's easier to
> figure what's going on. During the last test session I precisely had a
> build error that was quite annoying because once I managed to fix it I
> figured the fix was not the right one regarding other places.
>
> Alternately we could probably just add one line that always reports a
> failure like the other ones (it would be even better so that we can
> compare all outputs and still know that something fails):
>
>  +#else
>  +		CASE_TEST(__SIZEOF_LONG__defined);  EXPECT_EQ(1, 1, 0); break;
>
>> > +#endif /* __WORDSIZE == 64 */
>> 
>> #endif comment is now incorrect
>
> Good catch indeed!

Thanks: fixed in v5.

>> 
>> > +			case __LINE__:
>> 
>> The "case" should be further left, no?
>
> You're right!
>
> Thank you!
> Willy

Thank you for the review!