This is an archive of the discontinued LLVM Phabricator instance.

[C2x] Support the *_WIDTH macros in limits.h and stdint.h
ClosedPublic

Authored by aaron.ballman on Dec 7 2021, 8:15 AM.

Details

Summary

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.

Diff Detail

Event Timeline

aaron.ballman created this revision.Dec 7 2021, 8:15 AM
aaron.ballman requested review of this revision.Dec 7 2021, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 8:15 AM
Herald added a subscriber: aheejin. · View Herald Transcript

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?

aaron.ballman marked 2 inline comments as done.Dec 7 2021, 8:36 AM
aaron.ballman added inline comments.
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)?

aaron.ballman marked 2 inline comments as done.

Updated based on review feedback.

erichkeane added inline comments.Dec 7 2021, 8:43 AM
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
'#ifdef BOOL_WIDTH
#error
#endif`

And I'm not sure my relatively minor concerns are worth the effort.

erichkeane accepted this revision.Dec 14 2021, 6:29 AM
This revision is now accepted and ready to land.Dec 14 2021, 6:29 AM
aaron.ballman added a reviewer: joerg.

Updating based on off-list review feedback and adding a new reviewer.

Updating based on off-list review feedback and adding a new reviewer.

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.

jyknight added inline comments.Dec 14 2021, 9:53 AM
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?

aaron.ballman added inline comments.Dec 14 2021, 10:30 AM
clang/lib/Frontend/InitPreprocessor.cpp
900–903

This seems odd?

Yeah, this stuff is a bit twisty.

We define LONG_LONG_MAX but not LONG_LONG_WIDTH, for signed long long,

Correct.

and we define ULLONG_WIDTH but not ULLONG_MAX, for unsigned long long?

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.)

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).

aaron.ballman marked 2 inline comments as done.Dec 14 2021, 10:59 AM

Ping

Ping (I'd like to get this into Clang 14 as I have follow-up work that's related for _BitInt support in C23).

jyknight added inline comments.Jan 11 2022, 2:13 PM
clang/lib/Frontend/InitPreprocessor.cpp
900–903
  1. But, in this patch, it is exposing WIDTH for both variants of all the other types. Shouldn't we consistently expose WIDTH for only one of each pair (e.g. intmax_t, int_fast16_t, etc), instead of exposing both for some types, and one for others?
  1. Exposing it only for unsigned seems like the wrong choice and confusing, since that implies having a basically-extraneous "U" in all of the names, and is inconsistent with previous practice. Can we expose only the signed variants instead of the unsigned?

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.

jyknight added inline comments.Jan 12 2022, 9:53 AM
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
aaron.ballman added inline comments.Jan 12 2022, 10:14 AM
clang/lib/Frontend/InitPreprocessor.cpp
900–903

It's not clear to me why you made that change in response to my comments.

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.

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.

jyknight accepted this revision.Jan 13 2022, 8:03 AM

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.

aaron.ballman closed this revision.Jan 13 2022, 8:46 AM

Accepting assuming the last comment will be addressed before pushing. Thanks!

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.