erichkeane added inline comments.

================
Comment at: clang/lib/AST/ExprConstant.cpp:13533
+
+          if (NumNegativeBits) {
+            unsigned NumBits = std::max(NumNegativeBits, NumPositiveBits + 1);
----------------
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.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx2a.cpp:1477
+
+enum E { e1=-4, e2=4};
+void testValueInRangeOfEnumerationValues() {
----------------
erichkeane wrote:
> Theres likely quite a few more tests that need to be done.  We should have 
> one where it validates against a fixed underlying type, AND probably a pair 
> of ones to check against scoped enums.  
> 
> I wouldn't expect a scoped enum to diagnose, but a test to make sure we're 
> checking the right thing is likely valuable. 
Another good test is the empty-enum case.


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

https://reviews.llvm.org/D130058

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to