[PATCH][REVIEW REQUEST] Fix for libclang completion of already declared template friends.

[Francisco Lopes]

I'll put in words what I was trying to do in this fix. Sorry because it has
been so long that I'm reviewing it again.

Please check this for the reason of such fix:
http://llvm.org/bugs/show_bug.cgi?id=13699#c9

- I'm checking whether it's a friend because I still want to get the
friend's target as a result if it's ok to do so.

(The current codebase has an issue doing this as can be checked in the
issue tracker, and here I'm going for
the easier fix by changing the code completion source solely, which is
what's affecting me. I'm not sure whether
this friend lookup thing problem requires a major change in other parts of
the codebase, which it may, so
I opted for the minor fix that would solve this for code completion at
last.)

- The Decl::IDNS_Ordinary | Decl::IDNS_Tag | Decl::IDNS_Type check may be
spurious in the original call of
AddResult, but as can be seen, I'm gonna do a recursive call to it, and my
intent was to so that, at this second
direct call to AddResult, such check would be a valid one. The canonical
decl would be still marked as target
of friend decl, as I'm understanding, and I must check the visibility
because even though the non canonical friend
declaration may be visible, the canonical may be not, but the code would
still be valid. Like this one for example:

namespace boost {
    template<typename T> struct shared_ptr {
        template<typename U> friend struct unique_ptr; // 1
    };
}

int main() {
    boost::shared_ptr<int> p1;
    boost::| // 1 is visible, 2 not, so only boost::shared_ptr must show up
}

namespace boost {
    template<typename T> struct unique_ptr { // 2 canonical
    };
}



So the friend namespace check must exist, because of the current issue, its
intent is for applying such fix when it's the
case to do so, solely in lookups that involve friends.

And the visibility check exists because of the previous explanation.

I must get the canonical decl target of friending, so to suggest a
completion option with canonical parameters, and not the
ones of the friend declaration.

So,... in this process of thought, I still can't remove checks of the
patch. I hope to have explained it better.
Of course, this process of thought may be completely equivocated.

Regards,
Francisco



2014-04-14 22:53 GMT-03:00 Richard Smith <richard at metafoo.co.uk>:

> Hmm, I've looked into this a bit more, and it's unclear to me why we have
> an IDNS check here at all. If LookupVisibleDecls is working properly, it
> should only return declarations that are actually visible anyway.
>
> On Mon, Apr 14, 2014 at 1:15 PM, Francisco Lopes <
> francisco.mailing.lists at oblita.com> wrote:
>
>> Hi Richard, as can be checked in the bug history, there's a side effect
>> if I don't call getCanonicalDecl. Also, this patch fixes an issue that's
>> specially related to template friends and lookup. Please take a look at the
>> history, since it looks like following your advices, I would end up with
>> regressions.
>>
>>
>> 2014-04-11 17:10 GMT-03:00 Richard Smith <richard at metafoo.co.uk>:
>>
>> On Fri, Mar 14, 2014 at 6:59 AM, Francisco Lopes <
>>> francisco.mailing.lists at oblita.com> wrote:
>>>
>>>> hi, thanks for the comment but, for example, as I want to add the call
>>>> to getCanonicalDecl
>>>> for this situation of friends solely, don't I need to check whether
>>>> it's in friend name space too?
>>>>
>>>> I'm not sure whether you meant to replace the two first checks, or just
>>>> the second.
>>>>
>>>
>>> I meant to replace all the checks. I don't see why you would want to
>>> call getCanonicalDecl here, or why you'd care whether the name has ever
>>> been declared as a friend. All you should check is, is the name visible now?
>>>
>>> Regards.
>>>>
>>>>
>>>>
>>>> 2014-03-13 20:59 GMT-03:00 Richard Smith <richard at metafoo.co.uk>:
>>>>
>>>> Your visibility check seems more complex than necessary. I think this
>>>>> should do what you want:
>>>>>
>>>>>  if
>>>>> (ND->getMostRecentDecl()->isInIdentifierNamespace(Decl::IDNS_Ordinary |
>>>>> Decl::IDNS_Tag))
>>>>>   // visible
>>>>>
>>>>>
>>>>> On Wed, Mar 12, 2014 at 2:12 PM, Francisco Lopes <
>>>>> francisco.mailing.lists at oblita.com> wrote:
>>>>>
>>>>>> Ping
>>>>>>
>>>>>>
>>>>>> 2014-03-07 14:47 GMT-03:00 Francisco Lopes <
>>>>>> francisco.mailing.lists at oblita.com>:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> attached is a patch that tries to fix libclang bug 13699<http://llvm.org/bugs/show_bug.cgi?id=13699>
>>>>>>> .
>>>>>>> Please review.
>>>>>>>
>>>>>>> --
>>>>>>> Francisco Lopes
>>>>>>>
>>>>>>> PS:
>>>>>>> I have requested commit access in late 2012 but never made a test
>>>>>>> commit or anything.
>>>>>>>
>>>>>>> At the time I have received from Chris Lattner:
>>>>>>> "I'm sorry for the delay, I've been fighting mailing list issues.
>>>>>>> Commit after approval access is granted.  Please try a test commit!"
>>>>>>>
>>>>>>> I'm not sure whether it's still valid.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140416/62d5a02d/attachment.html>