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

Francisco Lopes francisco.mailing.lists at oblita.com
Wed Jan 14 18:44:41 PST 2015 

Much simpler, thanks!

2015-01-15 0:28 GMT-02:00 Richard Smith <richard at metafoo.co.uk>:

> 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/20150115/15130a58/attachment.html>

More information about the cfe-commits mailing list