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