Page MenuHomePhabricator

[ASTMatchers] has matcher matching as is
ClosedPublic

Authored by Prazek on May 30 2016, 3:39 PM.

Details

Summary

All tests passes. I used git-clang-format to format code, but I am not sure if it is totally legit right now (maybe running clang-format on whole file would change also above lines)

Diff Detail

Event Timeline

Prazek updated this revision to Diff 59003.May 30 2016, 3:39 PM
Prazek retitled this revision from to [ASTMatchers] has matcher matching as is.
Prazek updated this object.
Prazek added reviewers: klimek, alexfh, sbenza.
Prazek added subscribers: mnbvmar, staronj, krystyna, sbarzowski.
klimek edited edge metadata.May 31 2016, 12:59 AM

Thanks! Did you change all has() to has(ignoringParenImpCasts())? I wonder how many times we could remove the ignoringParenImpCasts without breaking any tests.

Not all - in unit tests I added it only to places where it was required.
To clang-tidy checks I added it everywhere, where inner matcher was expr.

BTW I am not sure if there should be some assertion for ignoringParenImpCast, when inner matcher is stmt. Right now it compiles, but of course
has(stmt()) doesnt match the same things as has(ignoringParenImpCast(stmt())), and it kinda doesn't make sense so I guess it should fail with compiler error.

Not all - in unit tests I added it only to places where it was required.
To clang-tidy checks I added it everywhere, where inner matcher was expr.

BTW I am not sure if there should be some assertion for ignoringParenImpCast, when inner matcher is stmt. Right now it compiles, but of course
has(stmt()) doesnt match the same things as has(ignoringParenImpCast(stmt())), and it kinda doesn't make sense so I guess it should fail with compiler error.

Why doesn't it make sense?

Not all - in unit tests I added it only to places where it was required.
To clang-tidy checks I added it everywhere, where inner matcher was expr.

BTW I am not sure if there should be some assertion for ignoringParenImpCast, when inner matcher is stmt. Right now it compiles, but of course
has(stmt()) doesnt match the same things as has(ignoringParenImpCast(stmt())), and it kinda doesn't make sense so I guess it should fail with compiler error.

Not all - in unit tests I added it only to places where it was required.
To clang-tidy checks I added it everywhere, where inner matcher was expr.

BTW I am not sure if there should be some assertion for ignoringParenImpCast, when inner matcher is stmt. Right now it compiles, but of course
has(stmt()) doesnt match the same things as has(ignoringParenImpCast(stmt())), and it kinda doesn't make sense so I guess it should fail with compiler error.

Why doesn't it make sense?

Maybe I don't understand it correctly, but I see in the documentation that ignoringParenImpCasts takes Matcher<Expr> as InnerMatcher, so I don't know what it even accept Matcher<Stmt>

Not all - in unit tests I added it only to places where it was required.
To clang-tidy checks I added it everywhere, where inner matcher was expr.

BTW I am not sure if there should be some assertion for ignoringParenImpCast, when inner matcher is stmt. Right now it compiles, but of course
has(stmt()) doesnt match the same things as has(ignoringParenImpCast(stmt())), and it kinda doesn't make sense so I guess it should fail with compiler error.

Not all - in unit tests I added it only to places where it was required.
To clang-tidy checks I added it everywhere, where inner matcher was expr.

BTW I am not sure if there should be some assertion for ignoringParenImpCast, when inner matcher is stmt. Right now it compiles, but of course
has(stmt()) doesnt match the same things as has(ignoringParenImpCast(stmt())), and it kinda doesn't make sense so I guess it should fail with compiler error.

Why doesn't it make sense?

Maybe I don't understand it correctly, but I see in the documentation that ignoringParenImpCasts takes Matcher<Expr> as InnerMatcher, so I don't know what it even accept Matcher<Stmt>

As Expr is a subclass of Stmt (yes, this is somewhat unexpected, and a long story), Matcher<Stmt> is a subclass of Matcher<Expr>; it is a reverse hierarchy.

This should be mentioned in the release notes, since it is a breaking change, lot of out of tree checks might be broken due to this one. And also it might be worth to consider those checks that utilizing the has matcher and did not break after the change. Maybe we were just "lucky" (or more like unlucky).

This should be mentioned in the release notes, since it is a breaking change, lot of out of tree checks might be broken due to this one. And also it might be worth to consider those checks that utilizing the has matcher and did not break after the change. Maybe we were just "lucky" (or more like unlucky).

That's why I have changed all ocurences of has matcher in clang-extra to has(ignoringParenImpCasts(. I will add note in release notes

Prazek updated this revision to Diff 59059.May 31 2016, 6:54 AM
Prazek edited edge metadata.

+ note in release notes

Gábor, you happy?

Gábor, you happy?

Sure, LGTM :)

klimek accepted this revision.May 31 2016, 8:28 AM
klimek edited edge metadata.

LG, thanks!

This revision is now accepted and ready to land.May 31 2016, 8:28 AM
Prazek closed this revision.May 31 2016, 8:41 AM

No problem, I like to make my job simpler :)