[PATCH] D108482: [Clang] Fix instantiation of OpaqueValueExprs (Bug #45964)

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/TreeTransform.h:10515-10525
+  // Note that SourceExpr can be nullptr.
+  ExprResult SourceExpr = TransformExpr(E->getSourceExpr());
+  if (SourceExpr.isInvalid())
+    return ExprError();
+  if (SourceExpr.get() == E->getSourceExpr() && !getDerived().AlwaysRebuild())
+    return E;
+
----------------
aaron.ballman wrote:
> These changes look correct to me, but I am not certain why they've been unnecessary for so long. This code is 11 years old and has never transformed the `OpaqueValueExpr` before (see https://github.com/llvm/llvm-project/commit/8d69a2160e48b73a4515c9401e67971fb8c14e07). @rjmccall, was this an oversight and we just got lucky for a long time, or is there a reason we didn't transform these expressions?
OVE always has to appear within a containing construct that has to handle it specially to preserve OVE equality.  It is likely that the structured bindings transform doesn't do that properly.  That should probably be fixed rather than trying to handle it here; creating a new OVE every time we encounter one is very problematic.  Perhaps this case should assert that it's unreachable.

For example, TreeTransform on PseudoObjectExpr recreates the syntactic form and then transforms that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108482/new/

https://reviews.llvm.org/D108482