[cfe-dev] [AST Matchers] has matcher bug?

[Manuel Klimek via cfe-dev]

Thanks! See comments on code review.

On Tue, May 31, 2016 at 12:41 AM Piotr Padlewski <piotr.padlewski at gmail.com>
wrote:

> http://reviews.llvm.org/D20801 please see review.
>
>
> Piotr
>
> 2016-05-30 23:30 GMT+02:00 Piotr Padlewski <piotr.padlewski at gmail.com>:
>
>> I don't see any objection, so I am taking care of this refactor right now.
>>
>> 2016-05-19 18:55 GMT+02:00 Manuel Klimek <klimek at google.com>:
>>
>>> So, I'm all for (2) unless we find people who object, and making people
>>> write has(ignoringParenImpCasts())
>>> That's already what we force people to do for various has* versions.
>>>
>>> On Thu, May 19, 2016 at 6:31 PM Piotr Padlewski <
>>> piotr.padlewski at gmail.com> wrote:
>>>
>>>> Sure, I assumed that the feature to "has" be equvalent to  has(ignoringParenImpCasts(x))
>>>> was important enough, to have new matcher, so you would not have to write
>>>> has(ignoringParenImpCasts(x)).
>>>>
>>>> 2016-05-19 17:10 GMT+02:00 Manuel Klimek <klimek at google.com>:
>>>>
>>>>> On Thu, May 19, 2016 at 3:48 PM Piotr Padlewski <
>>>>> piotr.padlewski at gmail.com> wrote:
>>>>>
>>>>>> The question is: in how many places the ignoring implicit casts is
>>>>>> needed for has? I am not sure about statistics, but if it would turned out
>>>>>> that in more than half places, the developer that wrote check didn't need
>>>>>> that, then I would probably go with option 2.
>>>>>>
>>>>>> For option 2 I would suggest to add another matcher that would be
>>>>>> equivalent to "has" that we have right now, but I am not sure how to call
>>>>>> it. "hasIndirect" would suggest that it only accepts indirect childs.
>>>>>>
>>>>>
>>>>> Why would we need a new matcher? We already have
>>>>> has(ignoringParenImpCasts(x)) which would be equivalent to today's has(x),
>>>>> don't we?
>>>>>
>>>>>> Anyway, I like option 2 more, because it seems more resonable.
>>>>>>
>>>>>>
>>>>>> Piotr
>>>>>>
>>>>>>
>>>>>> 17.05.2016 12:22 PM "Manuel Klimek" <klimek at google.com> napisał(a):
>>>>>>
>>>>>>> The problem is that in C++ you often have implicit conversions that
>>>>>>> are completely irrelevant.
>>>>>>> has() ignoring implicit conversions is more closely resembling
>>>>>>> what's written in the code.
>>>>>>>
>>>>>>> Both options:
>>>>>>> 1. adding a new function hasDirect
>>>>>>> 2. changing has() to not go through implicit conversions, and
>>>>>>> refactoring all uses of has() to has(ignoringParenImpCasts())
>>>>>>> ... seem fine to me, with different trade-offs.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>> /Manuel
>>>>>>>
>>>>>>> On Sun, May 15, 2016 at 3:06 PM Piotr Padlewski <
>>>>>>> piotr.padlewski at gmail.com> wrote:
>>>>>>>
>>>>>>>> Thanks for response Alexey.
>>>>>>>> Is there any reason why it do so? This is very unintuitive and it
>>>>>>>> also makes it harder and uglier to use matchers - instead of saying
>>>>>>>> something(has(something2())) I have to say
>>>>>>>> something2(hasParent(something())).
>>>>>>>>
>>>>>>>> Piotr
>>>>>>>>
>>>>>>>> 2016-05-15 13:38 GMT+02:00 Alexey Sidorin <alexey.v.sidorin at ya.ru>:
>>>>>>>>
>>>>>>>>> Hello Piotr,
>>>>>>>>>
>>>>>>>>> has() matcher ignores implicit casts and parens. That's not a bug
>>>>>>>>> (however, it will be good to point it in doxygen).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 13.05.2016 15:27, Piotr Padlewski via cfe-dev пишет:
>>>>>>>>>
>>>>>>>>> I am have a problem with has matcher. It doesn't work for cases
>>>>>>>>> like
>>>>>>>>> implicitCastExpr(has(implicitCastExpr()))
>>>>>>>>> or
>>>>>>>>>
>>>>>>>>> cxxMemberCallExpr(has(materializeTemporaryExpr())))
>>>>>>>>>
>>>>>>>>> or
>>>>>>>>>
>>>>>>>>> returnStmt(has(implicitCastExpr()))
>>>>>>>>>
>>>>>>>>> Here is a bug in bugzilla
>>>>>>>>>
>>>>>>>>> https://llvm.org/bugs/show_bug.cgi?id=27713
>>>>>>>>>
>>>>>>>>> Am I doing something wrong or it is a bug? This thing blocks me on 2 clang-tidy checks that I am working on.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> Piotr
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> cfe-dev mailing listcfe-dev at lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160531/af48da6a/attachment.html>