jyknight 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()));
----------------
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
```
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
