Feedback-ID: ic7094478:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-172-g9a2dae1853-fm-20230213.001-g9a2dae18
Mime-Version: 1.0
Message-Id: <09980a9f-8319-47b8-bb15-64ee0b52a6ed@app.fastmail.com>
In-Reply-To: <Y/J1RB1T4tFeBxgs@1wt.eu>
References: <20230219185133.14576-1-w@1wt.eu>
 <20230219185133.14576-5-w@1wt.eu>
 <c549eaa8-02c4-4ca5-9ad9-6b713e183609@t-8ch.de> <Y/J1RB1T4tFeBxgs@1wt.eu>
Date:   Mon, 20 Feb 2023 15:29:48 -0500
From:   "Vincent Dagonneau" <v@vda.io>
To:     "Willy Tarreau" <w@1wt.eu>,
        =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <thomas@t-8ch.de>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 4/4] tools/nolibc: add tests for the integer limits in stdint.h
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=C3=9Fschuh wrote:
>> > +#elif __SIZEOF_LONG__ =3D=3D 4
>> > +		CASE_TEST(limit_intptr_min);        EXPECT_EQ(1, INTPTR_MIN,  (i=
ntptr_t)  0x80000000); break;
>> > +		CASE_TEST(limit_intptr_max);        EXPECT_EQ(1, INTPTR_MAX,  (i=
ntptr_t)  0x7fffffff); break;
>> > +		CASE_TEST(limit_uintptr_max);       EXPECT_EQ(1, UINTPTR_MAX, (u=
intptr_t) 0xffffffffU); break;
>> > +		CASE_TEST(limit_ptrdiff_min);       EXPECT_EQ(1, PTRDIFF_MIN, (p=
trdiff_t) 0x80000000); break;
>> > +		CASE_TEST(limit_ptrdiff_max);       EXPECT_EQ(1, PTRDIFF_MAX, (p=
trdiff_t) 0x7fffffff); break;
>> > +		CASE_TEST(limit_ptrdiff_min);       EXPECT_EQ(1, PTRDIFF_MIN, (p=
trdiff_t) 0x80000000); break;
>> > +		CASE_TEST(limit_ptrdiff_max);       EXPECT_EQ(1, PTRDIFF_MAX, (p=
trdiff_t) 0x7fffffff); break;
>>=20
>> 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.=20

>> > +		CASE_TEST(limit_size_max);          EXPECT_EQ(1, SIZE_MAX,    (s=
ize_t)    0xffffffffU); break;
>> > +#else
>> > +# warning "__SIZEOF_LONG__ is undefined"
>>=20
>> 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 =3D=3D 64 */
>>=20
>> #endif comment is now incorrect
>
> Good catch indeed!

Thanks: fixed in v5.

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

Thank you for the review!