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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
