yaxunl added inline comments.
================
Comment at: clang/lib/Basic/IdentifierTable.cpp:111
KEYSYCL = 0x1000000,
+ KEYCUDA = 0x2000000,
KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
----------------
delcypher wrote:
> yaxunl wrote:
> > delcypher wrote:
> > > @yaxunl Is it intentional that you didn't update `KEYALL` here? That
> > > means `KEYALL` doesn't include the bit for `KEYCUDA`.
> > >
> > > If that was your intention then this will break if someone adds a new
> > > key. E.g.
> > >
> > > ```
> > > KEYCUDA = 0x2000000,
> > > KEYSOMENEWTHING = 0x4000000,
> > > // ...
> > > // KEYALL now includes `KEYCUDA`, whereas it didn't before.
> > > // KEYALL includes KEYSOMENEWTHING
> > > KEYALL = (0x7ffffff & ~KEYNOMS18 &
> > > ~KEYNOOPENCL) // KEYNOMS18 and KEYNOOPENCL are used to
> > > exclude.
> > > ...
> > > ```
> > >
> > >
> > > 1. Updating the `0x1ffffff` constant to `0x3ffffff` so that `KEYALL`
> > > includes `KEYCUDA`
> > > 2. If your intention **is** to not have `KEYCUDA` set in `KEYALL` then
> > > amend `KEYALL` to be.
> > >
> > > ```
> > > KEYALL = (0x7ffffff & ~KEYNOMS18 &
> > > ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are
> > > used to exclude.
> > > // KEYCUDA is not included in KEYALL
> > > ```
> > My intention is not to include KEYCUDA in KEYALL.
> >
> > Should I change KEYALL to
> >
> >
> > ```
> > KEYALL = (0x3ffffff & ~KEYNOMS18 &
> > ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are
> > used to exclude.
> > // KEYCUDA is not included in KEYALL
> > ```
> >
> > instead of
> >
> >
> > ```
> > KEYALL = (0x7ffffff & ~KEYNOMS18 &
> > ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are
> > used to exclude.
> > // KEYCUDA is not included in KEYALL
> > ```
> >
> > since the current maximum mask is 0x3ffffff instead of 0x7ffffff
> Oops, you're right it would be `0x3ffffff`. I wonder though if we should
> clean this up so we don't need to manually update the bit mask every time...
> what if it was written like this?
>
> ```lang=c++
> enum {
> KEYC99 = 0x1,
> KEYCXX = 0x2,
> KEYCXX11 = 0x4,
> ....
> KEYSYCL = 0x1000000,
> KEYCUDA = 0x2000000,
> KEYMAX = KEYCUDA, // Must be set to the largest KEY enum value
> KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
>
> // KEYNOMS18 and KEYNOOPENCL are used to exclude.
> // KEYCUDA is not included in KEYALL because <FIXME add reason here>
> KEYALL = (((KEYMAX & (KEYMAX-1)) & ~KEYNOMS18 & ~KEYNOOPENCL & ~KEYCUDA)
> };
> ```
On second thought, KEYALL does not need to exclude KEYCUDA.
However, it would be good to set KEYALL in a generic approach. I will open a
separate review.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124866/new/
https://reviews.llvm.org/D124866
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
