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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:620
+  case UO_Deref:  // *x
+  case UO_Not:    // ~x
+  case UO_Real:   // __real x
----------------
aaron.ballman wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > This is reachable and will always return true thanks to the magic of integer promotions:
> > > ```
> > > void func() {
> > >   bool b = true;
> > >   b = ~b;
> > > }
> > > ```
> > This code path is not just for boolean values though.
> All the more reason to be worried about `~` being marked as unreachable (I mostly worry because people often try to make bit fiddling operations constant expressions when possible, so this seems pretty likely to be hit). Perhaps make it into an `assert(false && "operation not supported yet");` or something more loud?
How is that different? It's just a marker for me so I know later in development that this is not implemented yet. And now that I'm writing this, I guess I see what you mean. Yes, it's not unreachable, but it shouldn't be reached when testing what is currently implemented.


================
Comment at: clang/lib/AST/Interp/Integral.h:173
+  }
+
   template <bool SrcSign> static Integral from(Integral<0, SrcSign> Value) {
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132098/new/

https://reviews.llvm.org/D132098