Date:   Sun, 19 Feb 2023 20:15:16 +0100
From:   Willy Tarreau <w@1wt.eu>
To:     Thomas =?iso-8859-1?Q?Wei=DFschuh?= <thomas@t-8ch.de>
Cc:     Vincent Dagonneau <v@vda.io>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 4/4] tools/nolibc: add tests for the integer limits
 in stdint.h
Message-ID: <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c549eaa8-02c4-4ca5-9ad9-6b713e183609@t-8ch.de>
Precedence: bulk

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.

> > +		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!
> 
> > +			case __LINE__:
> 
> The "case" should be further left, no?

You're right!

Thank you!
Willy