[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:13527
+          llvm::APInt Min;
+          llvm::APInt End;
+
----------------
Why "End" instead of max?


================
Comment at: clang/lib/AST/ExprConstant.cpp:13533
+
+          if (NumNegativeBits) {
+            unsigned NumBits = std::max(NumNegativeBits, NumPositiveBits + 1);
----------------
shafik wrote:
> erichkeane wrote:
> > erichkeane wrote:
> > > So my reading of 'within the range of the enumeration values' is different here.  Given:
> > > 
> > > `enum F { -4, 4};`, the valid values for assignment I would say are -4, -3, -2, -1, 0, 1, 2, 3, 4.  This error seems to be a bit more permissive here by making sure it is represent-able by the number of bits required to represent the enum.
> > > 
> > > 
> > Aaron pointed out off-line that I'm incorrect in what 'range of enumeration values' means, and that the values comes from https://eel.is/c++draft/dcl.enum#8.sentence-2.
> > 
> > So the logic here needs to be to find the smallest integer (regardless of the power-of-2-ness of its bit-size) that can represent all of the values (including a 1 bit value for empty I think?), and diagnose based on that.
> So I used ubsan to validate my checks e.g. https://godbolt.org/z/1j43465K7 but perhaps ubsan is getting it wrong as well?
Playing with that link, I THINK this is right?  I couldn't get it to be the 'wrong' message there. It DID take a while to figure out what is going on here.

I DO like the idea of abstracting this into some sort of `InRangeOfValues` thing on EnumDecl so that UBSan and this can share it.


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

https://reviews.llvm.org/D130058