Messages in this thread | | | Date | Mon, 20 Feb 2023 15:27:35 -0500 | From | "Vincent Dagonneau" <> | Subject | Re: [RFC PATCH 2/4] tools/nolibc: add integer types and integer limit macros |
| |
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!
| |