tbaeder added inline comments.
================
Comment at: clang/lib/AST/Interp/Integral.h:173
+ }
+
template <bool SrcSign> static Integral from(Integral<0, SrcSign> Value) {
----------------
erichkeane wrote:
> 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));
> }
> }
> ```
> }
> ```
>
That's a cool solution, thanks! And yes I'll push a renaming patch.
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
