Feedback-ID: ic7094478:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-172-g9a2dae1853-fm-20230213.001-g9a2dae18
Mime-Version: 1.0
Message-Id: <1cab0bc1-58b1-471f-97b6-8ff1995281f4@app.fastmail.com>
In-Reply-To: <20230220144732.GA15550@1wt.eu>
References: <20230219185133.14576-1-w@1wt.eu>
 <20230219185133.14576-3-w@1wt.eu>
 <4da90248dbe94c5db1036cd873dfd910@AcuMS.aculab.com>
 <20230220144732.GA15550@1wt.eu>
Date:   Mon, 20 Feb 2023 15:27:35 -0500
From:   "Vincent Dagonneau" <v@vda.io>
To:     "Willy Tarreau" <w@1wt.eu>,
        "David Laight" <David.Laight@ACULAB.COM>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 2/4] tools/nolibc: add integer types and integer limit macros
Content-Type: text/plain
Precedence: bulk

Hi David,

On Mon, Feb 20, 2023, at 09:47, Willy Tarreau wrote:
> Hi David,
>
> On Mon, Feb 20, 2023 at 09:14:04AM +0000, David Laight wrote:
>> From: Willy Tarreau
>> > Sent: 19 February 2023 18:52
>> > 
>> > This commit adds some of the missing integer types to stdint.h and adds
>> > limit macros (e.g. INTN_{MIN,MAX}).
>> > 
>> ...
>> > 
>> > +typedef   int8_t       int_least8_t;
>> > +typedef  uint8_t      uint_least8_t;
>> > +typedef  int16_t      int_least16_t;
>> > +typedef uint16_t     uint_least16_t;
>> > +typedef  int32_t      int_least32_t;
>> > +typedef uint32_t     uint_least32_t;
>> > +typedef  int64_t      int_least64_t;
>> > +typedef uint64_t     uint_least64_t;
>> 
>> The are also the 'fast' variants.
>> Although I'd be tempted to either not define the 'least'
>> or 'fast' types (or maybe define them all as 'long').
>> The only code I've ever seen that used uint_fast32_t
>> got 'confused' when it was 64 bits.
>
> Honestly I've never seen either the "least" nor the "fast" variants
> used and am not at all convinced we need them. But they're not causing
> issues either and I'm fine with Vincent adding them.
>

I have never seen them used in the wild either but I included them in the v5.

>> ...
>> > +/* limits of integral types */
>> > +
>> > +#define        INT8_MIN  (-128)
>> > +#define       INT16_MIN  (-32767-1)
>> > +#define       INT32_MIN  (-2147483647-1)
>> > +#define       INT64_MIN  (-9223372036854775807LL-1)
>> 
>> Those big decimal numbers are difficult to check!
>> A typo would be unfortunate!
>
> That's also the purpose of the test!
>

My rationale for writing the full decimal in the header file is that we have a check for that in the tests. Furthermore, I wrote the number in decimal there but in hexadecimal in the test. Hopefully, at least one of them is right and catches any mistake.

>> Maybe (eg):
>> #define	INT64_MIN	(-INT64_MAX - 1)
>
> Some would argue that it's less easy to check when you're grepping for
> a value. How often have you found yourself bouncing between glibc
> include files looking for a definition for example ? I'm not sold on
> either choice, it's indeed just a matter of taste in the end.
>
>> > +#define        INT8_MAX  (127)
>> > +#define       INT16_MAX  (32767)
>> > +#define       INT32_MAX  (2147483647)
>> > +#define       INT64_MAX  (9223372036854775807LL)
>> > +
>> > +#define       UINT8_MAX  (255)
>> > +#define      UINT16_MAX  (65535)
>> > +#define      UINT32_MAX  (4294967295U)
>> > +#define      UINT64_MAX  (18446744073709551615ULL)
>> 
>> None of those need brackets.
>
> Most likely it was done to be more uniform with the rest above.
>

It is mostly comestic, yes.

>> Defining in hex would be more readable.
>
> Sure they would but it's not the same. Hex numbers are usually
> considered as neither positive nor negative probably because they're
> more commonly used to manipulate bits rather than quantities, and often
> they will not trigger warnings on overflows. Look for example:
>
>   $ cat yy.c 
>   int a = 0x80000000;
>   int b = -0x80000000;
>   int c = 2147483648;
>   int d = -2147483648;
>
>   int e =  0x80000000 + 1;
>   int f =  0x80000000 - 1;
>   int g =  2147483648 + 1;
>   int h = -2147483648 - 1;
>  
>   $ clang  -W -Wall -Wextra -c yy.c
>   yy.c:3:9: warning: implicit conversion from 'long' to 'int' changes 
> value from 2147483648 to -2147483648 [-Wconstant-conversion]
>   int c = 2147483648;
>       ~   ^~~~~~~~~~
>   yy.c:8:21: warning: implicit conversion from 'long' to 'int' changes 
> value from 2147483649 to -2147483647 [-Wconstant-conversion]
>   int g =  2147483648 + 1;
>       ~    ~~~~~~~~~~~^~~
>   yy.c:9:21: warning: implicit conversion from 'long' to 'int' changes 
> value from -2147483649 to 2147483647 [-Wconstant-conversion]
>   int h = -2147483648 - 1;
>       ~   ~~~~~~~~~~~~^~~
>
> Notice how the hex ones didn't complain. Just for this I would
> rather keep the decimal ones, even if less readable.
>

Additionally, like previously mentioned, the tests are based on the hex representation as it is indeed easier to read.

>> Although all the 'f' get hard to count as well.
>> Given that the types are defined in the same file, why
>> not use ~0u and ~0ull for UINT32_MAX and UINT64_MAX.
>
> That's what I usually do but here I think it's mostly to stay
> consistent across the whole file.
>

Indeed, it is also mostly cosmetic.

>> Should UINT8_MAX and UINT16_MAX be unsigned constants?
>> (Or even be cast to the corresponding type?)
>
> Same, better not if we want to keep the compiler's warnings in case
> of wrong assignment. Just compare the outputs of:
>
>    char c = UINT8_MAX;
>
> when UINT8_MAX is defined as 255 and 255U. Only the former gives me:
>
>   yy.c:11:11: warning: implicit conversion from 'int' to 'char' changes 
> value from 255 to -1 [-Wconstant-conversion]
>   char cc = 255;
>        ~~   ^~~
>
> Thus it gives one extra opportunity to spot a typo.
>
> Thanks!
> Willy

Thank you for the review!