2016-04-26 0:55 GMT+06:00 Richard Smith <rich...@metafoo.co.uk>: > 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). > > Done. The content of `shouldLinkDependentDeclWithPrevious` now supports only the case of friend functions, more elaborated implementation will be made latter in separate changes.
> ================ > 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. > Indeed, without tracking of whether the declaration is a definition, the implementation becomes simpler. > ================ > 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 ... > Removed. > > ================ > 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). > > Removed. All job now is done by `shouldLinkDependentDeclWithPrevious`. http://reviews.llvm.org/D16989 > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits