Any feedback?
Thanks,
--Serge
2016-05-03 22:40 GMT+06:00 Serge Pavlov <sepavl...@gmail.com>:
> 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
