mysterymath added inline comments.
================ Comment at: clang/docs/ReleaseNotes.rst:43-45 +- The definition of ``USHRT_MAX`` in the freestanding ``<limits.h>`` no longer + overflows on AVR (where ``sizeof(unsigned int) == sizeof(unsigned short)``). + The type of ``USHRT_MAX`` on AVR is now ``unsigned int`` instead of ``int``, ---------------- aaron.ballman wrote: > I'd be surprised if this actually has much of a chance to break people. I'd > probably put this under a new AVR section under `Target Specific Changes`, > WDYT? SGTM; I was on the fence about this. ================ Comment at: clang/lib/Headers/limits.h:55 #define UCHAR_MAX (__SCHAR_MAX__*2 +1) -#define USHRT_MAX (__SHRT_MAX__ *2 +1) +#if __SHRT_MAX__ * 2U + 1U <= __INT_MAX__ +#define USHRT_MAX (__SHRT_MAX__ * 2 + 1) ---------------- aaron.ballman wrote: > Rather than do math here, WDYT about using the WIDTH macros? It seems like a > more direct comparison: > ``` > #if __SHRT_WIDTH__ == __INT_WIDTH__ > #endif > ``` > (This also makes me wonder if we have any targets where we need to do this > with `UCHAR_MAX` as well. IIRC, we only support `CHAR_BIT` == 8 and so we're > probably fine, but might as well double-check since we're fixing this kind of > mistake.) > Rather than do math here, WDYT about using the WIDTH macros? It seems like a > more direct comparison: Spent a few seconds proving always holds, and it does, so long as you have the 2's complement ranges. Given that the C standard formalized this in 2x, and it's all Clang supports anyway, SGTM. In Clang's TargetInfo, CharWidth is hard coded to 8, so there's no way to have a uchar that won't fit into int at present. I did add a test for this though in the limits.cpp, so if such a target were added to the list to check in the RUN lines, it would break. That being said, it probably *wouldn't* be added, as was the case with AVR; if AVR had run under that test, this issue would have been caught even with the original version at HEAD. ================ Comment at: clang/lib/Headers/limits.h:55 #define UCHAR_MAX (__SCHAR_MAX__*2 +1) -#define USHRT_MAX (__SHRT_MAX__ *2 +1) +#if __SHRT_MAX__ * 2U + 1U <= __INT_MAX__ +#define USHRT_MAX (__SHRT_MAX__ * 2 + 1) ---------------- mysterymath wrote: > aaron.ballman wrote: > > Rather than do math here, WDYT about using the WIDTH macros? It seems like > > a more direct comparison: > > ``` > > #if __SHRT_WIDTH__ == __INT_WIDTH__ > > #endif > > ``` > > (This also makes me wonder if we have any targets where we need to do this > > with `UCHAR_MAX` as well. IIRC, we only support `CHAR_BIT` == 8 and so > > we're probably fine, but might as well double-check since we're fixing this > > kind of mistake.) > > Rather than do math here, WDYT about using the WIDTH macros? It seems like > > a more direct comparison: > Spent a few seconds proving always holds, and it does, so long as you have > the 2's complement ranges. Given that the C standard formalized this in 2x, > and it's all Clang supports anyway, SGTM. > > In Clang's TargetInfo, CharWidth is hard coded to 8, so there's no way to > have a uchar that won't fit into int at present. I did add a test for this > though in the limits.cpp, so if such a target were added to the list to check > in the RUN lines, it would break. > That being said, it probably *wouldn't* be added, as was the case with AVR; > if AVR had run under that test, this issue would have been caught even with > the original version at HEAD. > Rather than do math here, WDYT about using the WIDTH macros? It seems like a > more direct comparison: > ``` > #if __SHRT_WIDTH__ == __INT_WIDTH__ > #endif > ``` > (This also makes me wonder if we have any targets where we need to do this > with `UCHAR_MAX` as well. IIRC, we only support `CHAR_BIT` == 8 and so we're > probably fine, but might as well double-check since we're fixing this kind of > mistake.) ================ Comment at: clang/lib/Headers/limits.h:55 #define UCHAR_MAX (__SCHAR_MAX__*2 +1) -#define USHRT_MAX (__SHRT_MAX__ *2 +1) +#define USHRT_MAX (__SHRT_MAX__ * 2U + 1U) #define UINT_MAX (__INT_MAX__ *2U +1U) ---------------- aaron.ballman wrote: > mysterymath wrote: > > aaron.ballman wrote: > > > mysterymath wrote: > > > > aaron.ballman wrote: > > > > > It's worth double-checking that this still gives the correct type for > > > > > the macro: > > > > > > > > > > C2x 5.2.4.2.1p2: For all unsigned integer types for which <limits.h> > > > > > or <stdint.h> define a macro with suffix _WIDTH holding its width N, > > > > > there is a macro with suffix _MAX holding the maximal value 2N − 1 > > > > > that is representable by the type and that has the same type as would > > > > > an expression that is an object of the corresponding type converted > > > > > according to the integer promotions. ... > > > > Ah, thanks; it hadn't occurred to me that the type of the expression > > > > would be specified in the standard. It could be either `unsigned int` > > > > or `int`, depending on the target. > > > > > > > > The most straightforward approach I could think of to produce the right > > > > type is: > > > > 1) Perform the arithmetic in `unsigned int` to produce the right value > > > > 2) Cast to `unsigned short` to produce the right type > > > > 3) Directly perform integer promotion using unary plus > > > > > > > > The use of unary plus is a bit odd here, but it seems like the most > > > > direct way to express the logic, and the overall expression seems less > > > > fragile than the `#if` alternative. I've added a comment to explain > > > > this as well. > > > Now the trouble is with the cast operation, because that runs afoul of > > > 5.2.4.2.1p1: The values given below shall be replaced by constant > > > expressions suitable for use in conditional expression inclusion > > > preprocessing directives. ... > > > > > > https://godbolt.org/z/K9cs66sdK > > > > > > I'm almost wondering if the most direct solution is for `__SHRT_MAX__` to > > > be generated with or without the `U` suffix depending on target. > > > > > > We should probably use this as an opportunity to add more exhaustive > > > testing for all the _MIN and _MAX macros to ensure the type properties > > > hold. I was playing around with something like this: > > > https://godbolt.org/z/o7KjY3asW > > > Now the trouble is with the cast operation, because that runs afoul of > > > 5.2.4.2.1p1: The values given below shall be replaced by constant > > > expressions suitable for use in conditional expression inclusion > > > preprocessing directives. ... > > > > > > https://godbolt.org/z/K9cs66sdK > > > > > Sigh, it appears my standards-fu is not strong. I'm rather surprised to > > find that you can't cast to an integer type in `#if`; 6.10.1.6 claim they > > take a `constant-expression`, evaluated according to 6.6. But I can't find > > anything in 6.6 that seems to exclude cast operators. The definition of > > integer constant expressions even specifically calls out casts to integer > > types, although it doesn't specifically say in 6.10.1.6 that it takes an > > integer constant expression. > > Would you happen to know off-hand what my mistake is? > > > > I think the `#if` version I had originally isn't subject to this; I'll > > switch back to that. > > > > > I'm almost wondering if the most direct solution is for `__SHRT_MAX__` to > > > be generated with or without the `U` suffix depending on target. > > Seems like that wouldn't work for the more straightforward case of `#define > > SHRT_MAX __SHRT_MAX__`; the type of this must always be int, since SHRT_MAX > > is always representable in int, even on 16-bit targets. > > > > > We should probably use this as an opportunity to add more exhaustive > > > testing for all the _MIN and _MAX macros to ensure the type properties > > > hold. I was playing around with something like this: > > > https://godbolt.org/z/o7KjY3asW > > Agree; I think a slight modification to the type test macros I added to the > > limits.cpp test can accomodate this. Seems like it would also be a good > > idea to verify 5.2.4.2.1p1 directly by using the values in the preprocessor. > > Sigh, it appears my standards-fu is not strong. > > No worries, this is what code review is intended to catch! :-) > > > I'm rather surprised to find that you can't cast to an integer type in #if; > > 6.10.1.6 claim they take a constant-expression, evaluated according to 6.6. > > But I can't find anything in 6.6 that seems to exclude cast operators. The > > definition of integer constant expressions even specifically calls out > > casts to integer types, although it doesn't specifically say in 6.10.1.6 > > that it takes an integer constant expression. > > Would you happen to know off-hand what my mistake is? > > The preprocessor has no notion of types, just numbers, identifiers, and > punctuation. Any identifier that isn't known to the preprocessor is replaced > by `0`, which is what allows code like this to compile: > https://godbolt.org/z/P3rzG8sxo > > So your previous macro would expand to `(+(0 0)(0x7FFF * 2U + 1U))` and the > preprocessor would get baffled. > > > Seems like that wouldn't work for the more straightforward case of #define > > SHRT_MAX __SHRT_MAX__; the type of this must always be int, since SHRT_MAX > > is always representable in int, even on 16-bit targets. > > Good point! > The preprocessor has no notion of types, just numbers, identifiers, and > punctuation. Any identifier that isn't known to the preprocessor is replaced > by `0`, which is what allows code like this to compile: > https://godbolt.org/z/P3rzG8sxo > > So your previous macro would expand to `(+(0 0)(0x7FFF * 2U + 1U))` and the > preprocessor would get baffled. Ah, the replacement by zero was what got me. It sounds from the wording in the standard that it's technically legal to have a cast in a constant expression in the preprocessor, but there's no way to actually *express* such a cast, since all the terms in it would be replaced by zeroes. Guess it saved them having to define a "preprocessor constant expression" notion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144218/new/ https://reviews.llvm.org/D144218 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
