<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2016-04-26 0:55 GMT+06:00 Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">rsmith added inline comments.<br>
<br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:8611-8612<br>
@@ -8609,3 +8610,4 @@<br>
} else {<br>
- // This needs to happen first so that 'inline' propagates.<br>
- NewFD->setPreviousDeclaration(cast<FunctionDecl>(OldDecl));<br>
+ if (NewFD->isOutOfLine() &&<br>
+ NewFD->getLexicalDeclContext()->isDependentContext() &&<br>
+ IsDefinition)<br>
----------------<br>
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.<br>
<br>
(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).<br>
<br></blockquote><div><br></div><div>Done. The content of `shouldLinkDependentDeclWithPrevious` now supports only the case of friend functions, more elaborated implementation will be made latter in separate changes.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
================<br>
Comment at: lib/Sema/SemaDecl.cpp:8613<br>
@@ +8612,3 @@<br>
+ NewFD->getLexicalDeclContext()->isDependentContext() &&<br>
+ IsDefinition)<br>
+ // Do not put friend function definitions found in template classes to<br>
----------------<br>
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.<br></blockquote><div><br></div><div>Indeed, without tracking of whether the declaration is a definition, the implementation becomes simpler.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:10951-10956<br>
@@ -10941,1 +10950,8 @@<br>
SkipBodyInfo *SkipBody) {<br>
+ // Don't complain if the given declaration corresponds to the friend function<br>
+ // declared in a template class. Such declaration defines the function only if<br>
+ // the template is instantiated, in the latter case the definition must be<br>
+ // found in corresponding class instantiation.<br>
+ if (FD->isOutOfLine() && FD->getLexicalDeclContext()->isDependentContext())<br>
+ return;<br>
+<br>
----------------<br>
Is this change necessary? If we're not putting dependent templates into redeclaration chains any more, we shouldn't find an existing definition ...<br></blockquote><div><br></div><div>Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:10962<br>
@@ -10945,3 +10961,3 @@<br>
if (!Definition)<br>
if (!FD->isDefined(Definition))<br>
return;<br>
----------------<br>
... down here.<br>
<br>
================<br>
Comment at: lib/Sema/SemaDeclCXX.cpp:12663-12673<br>
@@ -12662,1 +12662,13 @@<br>
<br>
+ // If a friend non-dependent function is declared in a dependent context, do<br>
+ // not put it into redeclaration chain of corresponding file level<br>
+ // declarations. Such function will be available when the template will be<br>
+ // instantiated.<br>
+ } else if (CurContext->isDependentContext() &&<br>
+ (D.getName().getKind() != UnqualifiedId::IK_TemplateId) &&<br>
+ (SS.isInvalid() || !SS.isSet())) {<br>
+ DC = CurContext;<br>
+ while (!DC->isFileContext())<br>
+ DC = DC->getParent();<br>
+ LookupName(Previous, S);<br>
+<br>
----------------<br>
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).<br>
<br></blockquote><div><br></div><div>Removed. All job now is done by `shouldLinkDependentDeclWithPrevious`.</div><div><br></div><div><a href="http://reviews.llvm.org/D16989" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16989</a></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
<br>
</blockquote></div><br></div></div>