[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:
> > > 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.


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

https://reviews.llvm.org/D115253