ahatanak added inline comments.
================
Comment at: lib/AST/ExprConstant.cpp:1165-1173
+ auto LB = Temporaries.lower_bound(Key);
+
+ // If an element with key Key is found, reset the value and return it. This
+ // can happen if Key is part of a default argument expression.
+ if (LB != Temporaries.end() && LB->first == Key)
+ return LB->second = APValue();
+
----------------
erik.pilkington wrote:
> I think that the problem is more subtle than this. This static assert errors
> (previously clang would assert) when it really it should be fine.
> ```
> constexpr const int &x(const int &p = 0) { return p; }
> static_assert(&x() != &x());
> ```
> Because default arguments are allocated on the caller side, both the calls to
> `x()` call createTemporary for the same MaterializeTemporaryExpr in the same
> CallStackFrame, when really that MTE should correspond to two distinct
> values. This patch just hides that underlying problem by recycling the value
> created during the first call during the second call.
>
> Maybe we could have a fancier key that incorporates a node on the caller
> side, such as the CXXDefaultArgExpr as well at the MTE, and store that fancy
> key in APValue::LValueBases? That would allow us generate distinct values for
> these MTEs, and also remember what expression originated it. What do you
> think about that?
>
> There is small discussion about this problem here:
> https://bugs.llvm.org/show_bug.cgi?id=33140
Thank you Erik for explaining the problem and pointing me to the PR.
I ended up using a version number that is updated every time
VisitCXXDefaultArgExpr is called. I initially tried using CXXDefaultArgExpr and
the void* pointer as the key, but discovered that that wouldn't fix the
assertion failure when compiling the first test case in PR33140 since
InitListExpr uses the same CXXConstructExpr (and hence the same
CXXDefaultArgExpr) to initialize the array.
Let me know if there are other cases I haven't thought about.
================
Comment at: test/SemaCXX/constexpr-default-arg.cpp:3
+
+// expected-no-diagnostics
+
----------------
lebedev.ri wrote:
> Down the line, it won't be obvious *what* this testcase is checking.
> At the very least wrap it into `namespace rdar_problem_36505742 {}`
I added comments that explain what it's trying to check.
https://reviews.llvm.org/D42776
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
