[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:13547-13551
+      if (auto ValD = Info.EvaluatingDecl.dyn_cast<const ValueDecl *>()) {
+        const VarDecl *VD = dyn_cast_or_null<VarDecl>(ValD);
+        if (VD && !VD->isConstexpr())
+          NotConstexprVar = true;
+      }
----------------
aaron.ballman wrote:
> This seems to be equivalent unless I'm misunderstanding something about the member dyn_cast.
I think the problem is that `PointerUnion` requires that it be one of the static types it was defined with and in this case that is `const ValueDecl *, const Expr *` but maybe I am missing something.


================
Comment at: clang/lib/AST/ExprConstant.cpp:13572-13575
         if (ED->getNumNegativeBits() &&
             (Max.slt(Result.getInt().getSExtValue()) ||
-             Min.sgt(Result.getInt().getSExtValue())))
-          Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-                                       diag::warn_constexpr_unscoped_enum_out_of_range)
-	       << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << Max.getSExtValue();
+             Min.sgt(Result.getInt().getSExtValue())) &&
+            !NotConstexprVar)
----------------
aaron.ballman wrote:
> Might as well test the easy condition before doing more work to get values and compare them.
> 
> I think we should rename `NotConstexprVar` to `ConstexprVar` so that we don't need the double negation here. WDYT?
Makes sense.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2424-2427
+E2 testDefaultArgForParam(E2 e2Param = (E2)-1) { // ok, not a constant expression context
+  E2 e2LocalInit = e2Param; // ok, not a constant expression context
+  return e2LocalInit;
+}
----------------
aaron.ballman wrote:
> Do you think it's worth it to add this as a test as well?
> ```
> consteval void testDefaultArgForParam2(E2 e2Param = (E2)-1) {
> }
> 
> void test() {
>   testDefaultArgForParam2(); // Make sure we get the error
>   testDefaultArgForParam2((E2)0); // Make sure we don't get the error
> }
> ```
It is a good idea but the diagnostic does not point to the line in `test()` which is unfortunate. 


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

https://reviews.llvm.org/D131874