[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/ByteCodeExprGen.cpp:620
+  case UO_Deref:  // *x
+  case UO_Not:    // ~x
+  case UO_Real:   // __real x
----------------
erichkeane wrote:
> tbaeder wrote:
> > 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.
> Aaron: we tend to NEVER use 'assert(false...' and instead use llvm_unreachable for 'not implemented yet' quite often in clang. I don't see the semantic difference here?
Aaron clarified offline: the problem is that llvm_unreachable inserts optimization hints in non-asserts builds, so the result is optimizer-caused-UB here.  The preference for 'assert(false', though it causing an odd state, at least doesn't allow the optimizer to go hog wild.

So I agree with Aaron here.

We DO do this wrong in some places, which is unfortunate, but should be considered mistakes, not precedent.


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

https://reviews.llvm.org/D132098