[PATCH] D121532: [Clang][WIP] Fix Unevaluated Lambdas

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

Adding Erich to look at this WIP for early feedback as he's recently been looking at template instantiation guts rather deeply, but I added some questions.



================
Comment at: clang/include/clang/AST/DeclCXX.h:280-282
+    LDKUnknown = 0,
+    LDKAlwaysDependent,
+    LDKNeverDependent,
----------------
(Matches the naming style used just above.)


================
Comment at: clang/lib/Sema/TreeTransform.h:12934
+  // substituting an unevaluated lambda inside of a function's parameter's type
+  // - as parameter type are not instanciated from within a function's DC. We
+  // use isUnevaluatedContext() to distinguish the function parameter case.
----------------



================
Comment at: clang/lib/Sema/TreeTransform.h:12948
+
+  if (OldClass->isDependentContext())
+    getDerived().transformedLocalDecl(OldClass, {Class});
----------------
Can you explain why you only do these transformations when the old class is a dependent context? Also, should this be looking at `getDerived().AlwaysRebuild()`? I'm not certain -- we're not calling any `Rebuild*` functions here because lambdas are not rebuilt but generated entirely from scratch. So I'm guessing we don't need to look at `AlwaysRebuild`, but it's a bit unclear.


================
Comment at: clang/lib/Sema/TreeTransform.h:12968-12971
+  if (E->getCallOperator()->isDependentContext()) {
+    getDerived().transformAttrs(E->getCallOperator(), NewCallOperator);
+    getDerived().transformedLocalDecl(E->getCallOperator(), {NewCallOperator});
+  }
----------------
Are these changes necessary as part of this patch? I'm a bit worried that only doing this in dependent contexts may change behavior for code like this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L769


================
Comment at: clang/lib/Sema/TreeTransform.h:13040
 
-      getDerived().transformedLocalDecl(OldVD, NewVDs);
+      if (OldVD->isTemplated())
+        getDerived().transformedLocalDecl(OldVD, NewVDs);
----------------
This also seems a bit surprising -- can't the variable declaration be non-templated but still dependent (e.g., an `auto` variable whose initialization is dependent?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121532