[PATCH] D115253: [C2x] Support the *_WIDTH macros in limits.h and stdint.h

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Frontend/InitPreprocessor.cpp:900-903
+  Builder.defineMacro("__USHRT_WIDTH__", Twine(TI.getShortWidth()));
+  Builder.defineMacro("__UINT_WIDTH__", Twine(TI.getIntWidth()));
+  Builder.defineMacro("__ULONG_WIDTH__", Twine(TI.getLongWidth()));
+  Builder.defineMacro("__ULLONG_WIDTH__", Twine(TI.getLongLongWidth()));
----------------
jyknight wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > aaron.ballman wrote:
> > > > jyknight wrote:
> > > > > 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?
> > > > > 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.
> > > 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?
> > > 
> > > 2. 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?
> > 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.
> 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
> ```
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115253/new/

https://reviews.llvm.org/D115253