[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/APValue.cpp:27
+    APValue::LValueBase Base;
+    bool IsOnePastTheEnd;
     CharUnits Offset;
----------------
Move this to the end so it can share space with `IsNullPtr`.


================
Comment at: lib/AST/ExprConstant.cpp:589-598
+    /// Keep track of the version of MTEs that are used by CXXDefaultArgExpr.
+    /// The version number is updated every time VisitCXXDefaultArgExpr is
+    /// called.
+    unsigned getDefaultArgNum() const { return CurDefaultArgNum; }
+    void setDefaultArgNum() {
+      assert(CurDefaultArgNum == 0 && "CurDefaultArgNum hasn't been cleared");
+      CurDefaultArgNum = ++NextDefaultArgNum;
----------------
This seems extremely similar to the purpose of the existing `CallIndex` parameter. Have you thought about ways the two might be unified?

If there's no reasonable path to unifying the two (which I suspect there may not be), it would make more sense to me to store a current version number on the `CallStackFrame` rather than globally in the `EvalInfo`, and to restart the numbering in each new call frame. That also emphasizes something else: this version number should be kept with the `CallIndex`. (We could achieve that by either moving the `CallIndex` into the `LValueBase` or moving the `Version` out of it.)


================
Comment at: lib/AST/ExprConstant.cpp:597
+    }
+    void clearDefaultArgNum() { CurDefaultArgNum = 0; }
+    unsigned CurDefaultArgNum = 0, NextDefaultArgNum = 0;
----------------
This is wrong: these scopes can nest, so you can't just reset the number to 0 when you're done. You should restore the prior number when you're done here, to divide up evaluation into CXXDefaultArgExpr scopes. (Technically, I think it would also be correct to leave the number alone when you leave one of these scopes, but only because a scope for a particular parameter's default argument can't nest within another scope for the same default argument expression -- and even that might not be true in the presence of template instantiation.)


================
Comment at: lib/AST/ExprConstant.cpp:4595
+  }
   bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *E) {
     // The initializer may not have been parsed yet, or might be erroneous.
----------------
We need the same handling for `CXXDefaultInitExpr`, too.


https://reviews.llvm.org/D42776