shafik added a comment.
Note I took my range generation code from `CGExpr.cpp` function
`getRangeForType(...)` I was considering perhaps making the code common, maybe
this could be a helper in `EnumDecl`? Does that makes sense?
================
Comment at: clang/lib/AST/ExprConstant.cpp:13512
+ if (Info.Ctx.getLangOpts().CPlusPlus20)
+ if (const EnumType *ET = dyn_cast<EnumType>(DestType)) {
----------------
erichkeane wrote:
> Why the check for C++20? This is UB as a Defect Report, which applies to all
> versions of C++.
I did not realized we apply defects back, I can remove the C++20 check.
================
Comment at: clang/lib/AST/ExprConstant.cpp:13533
+
+ if (NumNegativeBits) {
+ unsigned NumBits = std::max(NumNegativeBits, NumPositiveBits + 1);
----------------
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?
================
Comment at: clang/test/SemaCXX/constant-expression-cxx2a.cpp:1477
+
+enum E { e1=-4, e2=4};
+void testValueInRangeOfEnumerationValues() {
----------------
erichkeane wrote:
> 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.
Good point on the tests, yes we should not diagnose on a scoped enum.
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
