[PATCH] D133500: [clang] Correct handling of lambdas in lambda default arguments in dependent contexts.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2460-2475
-    FunctionDecl *OwningFunc = cast<FunctionDecl>(OldParm->getDeclContext());
-    if (OwningFunc->isInLocalScopeForInstantiation()) {
-      // Instantiate default arguments for methods of local classes (DR1484)
-      // and non-defining declarations.
-      Sema::ContextRAII SavedContext(*this, OwningFunc);
-      LocalInstantiationScope Local(*this, true);
-      ExprResult NewArg = SubstExpr(Arg, TemplateArgs);
----------------
This functionality is now incorporated into `Sema::SubstDefaultArguments()` and postponed until after construction of enclosing declaration contexts.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2592-2613
+  if (ForCallExpr) {
+    // Check the expression as an initializer for the parameter.
+    InitializedEntity Entity
+      = InitializedEntity::InitializeParameter(Context, Param);
+    InitializationKind Kind = InitializationKind::CreateCopy(
+        Param->getLocation(),
+        /*FIXME:EqualLoc*/ PatternExpr->getBeginLoc());
----------------
I find this code exceedingly troubling, but I haven't been able to figure out how to unify it. Conceptually, instantiation of a default argument should produce the same result (the instantiated default argument is cached after all!) whether it is implicitly always instantiated (as in a declaration in local scope or a friend function definition) or instantiated on demand for a call expression. However, my attempts to unify this code have all resulted in test failures that I have, so far, been unable to explain. Note that much of the code in the `ForCallExpr` case is replicated in the `ConvertParamDefaultArgument()` function. I think this should be investigated further and may be a reason to postpone approving this change.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2138
+  else if (D->isLocalExternDecl()) {
+    LexicalDC = SemaRef.CurContext;
+  }
----------------
This seems highly questionable to me. I suspect there is a better way to identify the correct context, but I have not so far found it elsewhere. Note that the context needed is for the enclosing function template specialization (not the enclosing template).


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2296-2311
+    for (ParmVarDecl *PVD : Function->parameters()) {
+      if (!PVD->hasDefaultArg())
+        continue;
+      if (SemaRef.SubstDefaultArgument(D->getInnerLocStart(), PVD, TemplateArgs)) {
+        // If substitution fails, the default argument is set to a
+        // RecoveryExpr that wraps the uninstantiated default argument so
+        // that downstream diagnostics are omitted.
----------------
This code is effectively replicated in three distinct locations. It is probably worth moving it into a helper function somewhere.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4565
-                          /*DiscardedValue*/ false);
-  if (Result.isInvalid())
     return true;
----------------
The above removed code has been incorporated into `Sema::SubstDefaultArgument()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133500