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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
