[PATCH] D132098: [clang][Interp] Implement inv and neg unary operations

[Erich Keane via Phabricator via cfe-commits]

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