[PATCH][REVIEW REQUEST] Fix for libclang completion of already declared template friends.
Francisco Lopes
francisco.mailing.lists at oblita.com
Wed Apr 16 09:41:30 PDT 2014
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/20140416/f2ba9111/attachment.html>
More information about the cfe-commits
mailing list