Re: [PATCH] D23493: Fix PR28366: Teach the const-expression evaluator to be more fault tolerant with non-const enclosing local variables, or otherwise fold them if const.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/AST/ExprConstant.cpp:4795-4797
@@ +4794,5 @@
+  if (VD->hasLocalStorage() && Info.CurrentCall->Index > 1) {
+    // Only if the DeclContext of VD is the same as the called function do we
+    // set the Frame to the Current CallStackFrame, so that we can find its
+    // associated value from the variable-objects associated with that frame.
+    if (Info.CurrentCall->Callee && isa<FunctionDecl>(VD->getDeclContext()) &&
----------------
This is talking too much about the what and not enough about the why. "We 
expect to find a value for a local variable in the current frame, unless the 
variable was actually declared in a lexically-enclosing context." or something 
like that?

================
Comment at: lib/AST/ExprConstant.cpp:4798-4800
@@ +4797,5 @@
+    // associated value from the variable-objects associated with that frame.
+    if (Info.CurrentCall->Callee && isa<FunctionDecl>(VD->getDeclContext()) &&
+        cast<FunctionDecl>(VD->getDeclContext()->getRedeclContext())
+                ->getFirstDecl() == Info.CurrentCall->Callee->getFirstDecl()) {
+      Frame = Info.CurrentCall;
----------------
This can be specified more simply as:

  if (Info.CurrentCall->Callee &&
      Info.CurrentCall->Callee->Equals(VD->getDeclContext()))

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:190
@@ -189,3 +189,3 @@
   if (A->getCond()->isValueDependent() && !Cond->isValueDependent() &&
-      !Expr::isPotentialConstantExprUnevaluated(Cond, cast<FunctionDecl>(Tmpl),
+      !Expr::isPotentialConstantExprUnevaluated(Cond, cast<FunctionDecl>(New),
                                                 Diags)) {
----------------
Do we have existing test coverage for this (tests that fail with the 
ExprConstant change if we don't fix this at the same time)?

================
Comment at: test/SemaCXX/constant-expression-cxx11.cpp:2091
@@ +2090,3 @@
+  };
+  constexpr int CD = X::f();
+}
----------------
Maybe also `static_assert` that we get the correct value here?


https://reviews.llvm.org/D23493



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