[PATCH] D16989: Change interpretation of function definition in friend declaration of template class.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/Sema/SemaDecl.cpp:8611-8612
@@ -8609,3 +8610,4 @@
     } else {
-      // This needs to happen first so that 'inline' propagates.
-      NewFD->setPreviousDeclaration(cast<FunctionDecl>(OldDecl));
+      if (NewFD->isOutOfLine() &&
+          NewFD->getLexicalDeclContext()->isDependentContext() &&
+          IsDefinition)
----------------
Please factor this check out into a suitably-named function, `shouldLinkDependentDeclWithPrevious` or similar, and pass in `OldDecl` as well. I imagine we'll want to call this from multiple places (for instance, when handling `VarDecl`s), and I can see a few ways of making it return `true` in more cases, which would allow us to provide more precise diagnostics in a few more cases.

(For instance, if the old and new declarations are in the same lexical context, we can mark them as redeclarations, and more generally I think we can do so if the new declaration has no more template parameters in scope than the old one did and the old declaration is declared within the current instantiation of the new declaration).

================
Comment at: lib/Sema/SemaDecl.cpp:8613
@@ +8612,3 @@
+          NewFD->getLexicalDeclContext()->isDependentContext() &&
+          IsDefinition)
+        // Do not put friend function definitions found in template classes to
----------------
I don't think it makes sense to check whether the template declaration is a definition. It should not be added to the redeclaration chain regardless of whether it's a definition.

================
Comment at: lib/Sema/SemaDecl.cpp:10951-10956
@@ -10941,1 +10950,8 @@
                                    SkipBodyInfo *SkipBody) {
+  // Don't complain if the given declaration corresponds to the friend function
+  // declared in a template class. Such declaration defines the function only if
+  // the template is instantiated, in the latter case the definition must be
+  // found in corresponding class instantiation.
+  if (FD->isOutOfLine() && FD->getLexicalDeclContext()->isDependentContext())
+    return;
+
----------------
Is this change necessary? If we're not putting dependent templates into redeclaration chains any more, we shouldn't find an existing definition ...

================
Comment at: lib/Sema/SemaDecl.cpp:10962
@@ -10945,3 +10961,3 @@
   if (!Definition)
     if (!FD->isDefined(Definition))
       return;
----------------
... down here.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:12663-12673
@@ -12662,1 +12662,13 @@
 
+  // If a friend non-dependent function is declared in a dependent context, do
+  // not put it into redeclaration chain of corresponding file level
+  // declarations. Such function will be available when the template will be
+  // instantiated.
+  } else if (CurContext->isDependentContext() &&
+             (D.getName().getKind() != UnqualifiedId::IK_TemplateId) &&
+             (SS.isInvalid() || !SS.isSet())) {
+    DC = CurContext;
+    while (!DC->isFileContext())
+      DC = DC->getParent();
+    LookupName(Previous, S);
+
----------------
What do these changes have to do with avoiding putting the declaration into the redeclaration chain? This looks equivalent to what the following `if` block will do, except that (a) it uses `LookupName` instead of `LookupQualifiedName` (which might be a bug fix but doesn't seem related to whether the context was dependent), and (b) it forgets to set `DCScope` (which looks like a bug).


http://reviews.llvm.org/D16989