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 < [email protected]>: > 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 <[email protected]>: > > 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 < >> [email protected]> 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 <[email protected]>: >>> >>> On Fri, Mar 14, 2014 at 6:59 AM, Francisco Lopes < >>>> [email protected]> 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 <[email protected]>: >>>>> >>>>> 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 < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Ping >>>>>>> >>>>>>> >>>>>>> 2014-03-07 14:47 GMT-03:00 Francisco Lopes < >>>>>>> [email protected]>: >>>>>>> >>>>>>>> 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 >>>>>>> [email protected] >>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
