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

Francisco Lopes francisco.mailing.lists at oblita.com
Mon Jan 5 16:29:33 PST 2015 

ping

2014-12-04 7:09 GMT-02:00 Manuel Klimek <klimek at google.com>:

> Richard, according to the YCM owner, more people hit this. Is there a
> problem with the patch? Anything else we need to do to drive this forward?
>
> Thanks,
> /Manuel
>
>
> On Wed Apr 30 2014 at 10:05:18 PM Francisco Lopes <
> francisco.mailing.lists at oblita.com> wrote:
>
>> PING.
>>
>> Does my previous statements make any sense?
>> I'm not so into this patch anymore, but locally at my machine, it solves
>> the issue without side-effects.
>> I believe some measure regarding this issue could be taken, given all the
>> information I've given.
>>
>> Regards.
>>
>>
>>
>> 2014-04-16 13:41 GMT-03:00 Francisco Lopes <
>> francisco.mailing.lists at oblita.com>:
>>
>> Reposting, since last one looks like didn't formatted well:
>>>
>>> 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-16 13:28 GMT-03:00 Francisco Lopes <
>>> francisco.mailing.lists at oblita.com>:
>>>
>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>  _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> _______________________________________________
> 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/20150105/a5e75ea2/attachment.html>

More information about the cfe-commits mailing list