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

[Serge Pavlov via cfe-commits]

Any feedback?

Thanks,
--Serge

2016-05-03 22:40 GMT+06:00 Serge Pavlov <sepavloff at gmail.com>:

> 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/20160516/d91c9718/attachment-0001.html>