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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to