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.

cfe-commits mailing list

Reply via email to