Page MenuHomePhabricator

[ASTMatchers] New matcher hasReturnValue added
ClosedPublic

Authored by baloghadamsoftware on Mar 9 2016, 4:34 AM.

Details

Summary

A checker (will be uploaded after this patch) needs to check implicit casts. Existing generic matcher "has" ignores implicit casts and parenthesized expressions and no specific matcher for matching return value expression preexisted. The patch adds such a matcher (hasReturnValue).

Diff Detail

Repository
rL LLVM

Event Timeline

baloghadamsoftware retitled this revision from to [ASTMatchers] Existing matcher hasAnyArgument fixed and new matcher hasReturnValue added.
baloghadamsoftware updated this object.
baloghadamsoftware added a reviewer: klimek.
baloghadamsoftware added a subscriber: cfe-commits.
sbenza edited edge metadata.Mar 11 2016, 7:57 AM

The reason we haven't fixed hasAnyArgument is that it can potentially break its users.
I'd prefer if you separated the fix from the addition.
That way we can revert the fix if needed.

include/clang/ASTMatchers/ASTMatchers.h
4796 ↗(On Diff #50120)

New matchers must be tested.
See ASTMatchersTest.cpp.

Also, they should be added to the dynamic registry.
See Dynamic/Registry.cpp

4800 ↗(On Diff #50120)

I'm not sure this is needed.
returnStmt(hasReturnValue(...)) is equivalent to returnStmt(has(...))
There are many checks using the latter already.

The reason we haven't fixed hasAnyArgument is that it can potentially break its users.
I'd prefer if you separated the fix from the addition.
That way we can revert the fix if needed.

I will separate it, OK. In the Clang there is one use case that I fixed, although it did not break the tests. Neither of the other "has..." checkers (except the general ones) ignore implicit casts and parenthesized expressions so this one should not do it either because it makes checking implicit casts impossible.

Matcher "hasReturnValue" is needed because "has" ignores all implicit casts and parenthesized expressions and we need to the check implicit casts. I will add a test and add it to the dynamic registry.

I will separate it, OK. In the Clang there is one use case that I fixed, although it did not break the tests. Neither of the other "has..." checkers (except the general ones) ignore implicit casts and parenthesized expressions so this one should not do it either because it makes checking implicit casts impossible.

I agree that we want it, just wanted to point out that it has to be done with care.

Matcher "hasReturnValue" is needed because "has" ignores all implicit casts and parenthesized expressions and we need to the check implicit casts. I will add a test and add it to the dynamic registry.

I see. has() suffers from the same problem. Then it makes sense.

baloghadamsoftware retitled this revision from [ASTMatchers] Existing matcher hasAnyArgument fixed and new matcher hasReturnValue added to [ASTMatchers] New matcher hasReturnValue added.
baloghadamsoftware updated this object.
baloghadamsoftware edited edge metadata.

Separation of new matcher and fix for existing matcher. This patch only contains the new matcher. Test added and matcher added to the registry.

Previous patch generation failed.

sbenza added inline comments.Mar 18 2016, 12:40 PM
include/clang/ASTMatchers/ASTMatchers.h
4848 ↗(On Diff #50932)

a + b (with spaces)
A few in this comment, and one in the test.

4854 ↗(On Diff #50932)

Wouldn't this just be:

AST_MATCHER_P(...) {
  return InnerMatcher.matches(*Node.getRetValue(), Finder, Builder);
}
sbenza accepted this revision.Mar 21 2016, 7:56 AM
sbenza edited edge metadata.
This revision is now accepted and ready to land.Mar 21 2016, 7:56 AM

Can you rerun the doc script (dump_ast_matchers.py)?

I can rerun the script, however it seems it was not executed before the last commit on the master branch, thus if I rerun it then changes will appear in my diff which are not related to my work. What is the exect policy about running this scipt? Should it be rerun before the commit (then it was forgotten by the last committer) or it is executed only before releases?

I can rerun the script, however it seems it was not executed before the last commit on the master branch, thus if I rerun it then changes will appear in my diff which are not related to my work. What is the exect policy about running this scipt? Should it be rerun before the commit (then it was forgotten by the last committer) or it is executed only before releases?

I don't think there is a specific policy here. We try to keep it up to date, but some changes forget to do it.
We can do it in a separate patch if you feel the extra diff should not be on this one.

baloghadamsoftware edited edge metadata.

LibASTMatchersReference.html regenerated

This revision was automatically updated to reflect the committed changes.