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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
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;
----------------
rsmith wrote:
> 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.)
It couldn't come up with a way to unify CallIndex and Version. I think it's better to keep them separate because you need the CallIndex to get the frame that owns the temporary and you need Version to distinguish between two temporaries created by the same MTE.

I moved CallIndex into LValueBase as you suggested.


https://reviews.llvm.org/D42776