aaron.ballman added reviewers: jyknight, clang-language-wg. aaron.ballman 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``, ---------------- 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? ================ 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) ---------------- 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) ---------------- 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! 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
