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

[Serge Pavlov via cfe-commits]

2016-04-26 0:55 GMT+06:00 Richard Smith <richard at 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

>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160503/5b813407/attachment.html>