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

Francisco Lopes francisco.mailing.lists at oblita.com
Sun Jul 20 21:22:11 PDT 2014 

ping


2014-04-30 17:03 GMT-03:00 Francisco Lopes <
francisco.mailing.lists at oblita.com>:

> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140721/9dd83023/attachment.html>

More information about the cfe-commits mailing list