This completes the implementation of WG14 N2412 (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf), which standardizes C on a twos complement representation for integer types. The only work that remained there was to define the correct macros in the standard headers, which this patch does.
Details
Details
Diff Detail
Diff Detail
Unit Tests
Unit Tests
Event Timeline
Comment Actions
A few nits, but not nearly 'expert' enough to approve without giving everyone else some time to look this over.
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 269 | Minor-est of nits: It would seem that TypeSize and TypeWidth should go together, just because of how similar they are. Is there a reason to not put this line above the Fmt? | |
| clang/test/Headers/limits.cpp | ||
| 34–35 | Is this change related? Not sure I get what the change here is. | |
| 63 | Hmm... this is somewhat clever... perhaps overly so? Can you improve the comment on 61? | |
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 269 | No reason not to move it, so move it I shall. | |
| clang/test/Headers/limits.cpp | ||
| 34–35 | The original code would only compile in C++ mode; once I added a C RUN line, it failed because it wasn't a valid constant expression in C. This change makes it a valid constant expression in both C and C++ mode. | |
| 63 | FWIW, this follows the same pattern as what's on line 41. Do you have a suggested improvement (I can make it in both spots)? | |
| clang/test/Headers/limits.cpp | ||
|---|---|---|
| 34–35 | Ah, thanks for the clarification! | |
| 63 | Hrmph, so prior art here :( It annoys me a touch that it ONLY works to ensure that they are not defined to something that would be an illegal identifier (so accidentally only defining INT_WIDTH and not UINT_WIDTH wouldn't be caught). BUT I don't have a better idea other than some silliness of And I'm not sure my relatively minor concerns are worth the effort. | |
Comment Actions
This added DefineTypeSizeAndWidth() as a helper for setting the _MAX and _WIDTH macros as a pair and it introduces some more testing for the width's relationship to sizeof and CHAR_BIT.
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 900–903 | This seems odd? We define __LONG_LONG_MAX__ but not __LONG_LONG_WIDTH__, for signed long long, and we define __ULLONG_WIDTH__ but not __ULLONG_MAX__, for unsigned long long? | |
| clang/lib/Headers/stdint.h | ||
| 795 | Why are these conditioned on __STDC_VERSION__ but the ones above, e.g. UINT64_WIDTH, are not? | |
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 900–903 |
Yeah, this stuff is a bit twisty.
Correct.
Correct. The basic approach I took was: for types with signed/unsigned pairs, define the unsigned variants with an underscore name and use them to define the signed variants in the header file because the width of signed and unsigned has to be the same per spec ("The macro blah_WIDTH represents the width of the type blah and shall expand to the same value as Ublah_WIDTH.") So that's why you generally only see the "unsigned" width variants added here with a double underscore name. I could expose the signed versions as well (like we do for many of the MAX macros), but that makes the preprocessor slower for no real benefit as we want the users to use the non-underscore versions of these macros whenever possible. | |
| clang/lib/Headers/stdint.h | ||
| 795 | Good catch, that's a think-o on my part! And it's even a think-o in the test files! #if defined(INT64_MAX) _Static_assert(INT64_WIDTH == 64, ""); _Static_assert(UINT64_WIDTH == INT64_WIDTH, ""); _Static_assert(INT64_WIDTH / __CHAR_BIT__ == sizeof(int64_t), ""); _Static_assert(UINT64_WIDTH / __CHAR_BIT__ == sizeof(uint64_t), ""); #else int INT64_WIDTH, UINT64_WIDTH; /* None of these are defined. */ #endif That predicate is wrong; it should be testing the standard version *and* the presence of the type, not just the presence of the type alone. (We always have the 8, 16, 32, and 64 types, so the test for "none of these are defined" is never run.) | |
Comment Actions
Updated based on review feedback. This protects all of the _WIDTH macros in stdint.h, which was a previous oversight. It also corrects the test case to actually test the scenario we care about (which requires that we look at the standard version).
Comment Actions
Ping (I'd like to get this into Clang 14 as I have follow-up work that's related for _BitInt support in C23).
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 900–903 |
| |
Comment Actions
Updating based on review feedback -- adds the signed versions of the macros for consistency and uses them in limits.h.
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 900–903 | Ah, I see what you mean now, thanks for pushing back. Yes, I can add the signed variants here so we expose both the signed and unsigned versions for everything consistently. I'll also switch the headers to use both the signed and unsigned variants as appropriate -- the tests ensure these values won't get out of sync. | |
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 900–903 | It's not clear to me why you made that change in response to my comments. You gave a good rationale before as to why we don't want to have both signed and unsigned versions of the macros, so the change I was trying to suggest in my last comment is to consistently expose only the signed variants for __*_WIDTH__, and not the unsigned variants. For comparison, gcc 11.2, gcc -E -dM -xc /dev/null |grep WIDTH|sort gives: #define __INT_FAST16_WIDTH__ 64 #define __INT_FAST32_WIDTH__ 64 #define __INT_FAST64_WIDTH__ 64 #define __INT_FAST8_WIDTH__ 8 #define __INT_LEAST16_WIDTH__ 16 #define __INT_LEAST32_WIDTH__ 32 #define __INT_LEAST64_WIDTH__ 64 #define __INT_LEAST8_WIDTH__ 8 #define __INTMAX_WIDTH__ 64 #define __INTPTR_WIDTH__ 64 #define __INT_WIDTH__ 32 #define __LONG_LONG_WIDTH__ 64 #define __LONG_WIDTH__ 64 #define __PTRDIFF_WIDTH__ 64 #define __SCHAR_WIDTH__ 8 #define __SHRT_WIDTH__ 16 #define __SIG_ATOMIC_WIDTH__ 32 #define __SIZE_WIDTH__ 64 #define __WCHAR_WIDTH__ 32 #define __WINT_WIDTH__ 32 | |
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 900–903 |
I had the impression you wanted both signed and unsigned _WIDTH macros to be emitted for consistency with the exact-width (et al) macros. Guess it was the opposite, you want the exact-width and basic macros to only define the signed versions. However, this makes use of DefineTypeSizeAndWidth() within DefineExactWidthIntTypeSize() and similar kind of awkward -- that now has to track whether we're defining a signed vs unsigned type to decide whether to emit the _WIDTH macro. So I guess that'll have to be reworked. | |
Comment Actions
Reworked a different way -- expose the signed versions of the width macros rather than the unsigned versions or both versions. Note, __UINTPTR_WIDTH__ and __UINTMAX_WIDTH__ were both preexisting macros, so I did not remove them.
Comment Actions
Accepting assuming the last comment will be addressed before pushing. Thanks!
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 258 | DefineTypeSizeAndWidth isn't necessary from DefineExactWidthIntTypeSize, since __INT#_WIDTH__ == # always, so there's no point in emitting #define __INT8_WIDTH__ 8, etc. | |
Comment Actions
Thank you for the excellent reviews, I really appreciate it!
I made the requested changes and commit in bf7d9970ba0ac5ecfa1a469086f5789de5c94e3f.
| clang/lib/Frontend/InitPreprocessor.cpp | ||
|---|---|---|
| 258 | LOL, good catch. | |
DefineTypeSizeAndWidth isn't necessary from DefineExactWidthIntTypeSize, since __INT#_WIDTH__ == # always, so there's no point in emitting #define __INT8_WIDTH__ 8, etc.