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

Francisco Lopes francisco.mailing.lists at oblita.com
Wed Apr 30 13:03:31 PDT 2014 

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/20140430/4169578b/attachment.html>

More information about the cfe-commits mailing list