erichkeane added inline comments.
================
Comment at: clang/lib/AST/Interp/Integral.h:173
+ }
+
template <bool SrcSign> static Integral from(Integral<0, SrcSign> Value) {
----------------
aaron.ballman wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > tbaeder wrote:
> > > > I'm a bit out of my element with the template magic here.
> > > It mostly looks correct to me, but the part I'm struggling with is
> > > `static_cast<Integral::T>`; can you explain what you're trying to do
> > > there?
> > I was trying to implement creating an Integral from a Boolean; the
> > `static_cast` casts to `Integral::T` (which in my case is `int`). It might
> > not be needed.
> Oh!!! I see now why there's so much confusion. You have `template <typename
> T>` and I was wondering why you were trying to use that here. I see now that
> there's `Integral::T` as a private member.
>
> Can we please rename `Integral::T` to be something mildly descriptive? (I'm
> fine if that's done in a follow-up as an NFC change.)
As Aaron said separately, the 'T' name of the integral member is bonkers and
confusing!
So it appears the intent here is to take a non-integral 'Value', cast it to the
underlying representation, and then call the version of this on 159.
From a 'style' perspective, I'd prefer this live right next to that version
(since it is the inverse SFINAE). In fact, I'd probably prefer you switch this
to an 'if constexpr' instead:
```
template <typename ValTy>
static Integral from (ValTy Value) {
if constexpr (std::is_integral_v<T>) {
return Integral(Value);
} else {
return Integral(static_cast<Integral::RepTy>(Value));
}
}
```
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132098/new/
https://reviews.llvm.org/D132098
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
