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

Richard Smith richard at metafoo.co.uk
Wed Jan 14 18:28:49 PST 2015 

Fixed in a simpler way in r226083.

On Mon, Jan 5, 2015 at 4:29 PM, Francisco Lopes <
francisco.mailing.lists at oblita.com> wrote:

> 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/20150114/00f56533/attachment.html>

More information about the cfe-commits mailing list