Add an AST matcher for CastExpr kind
ClosedPublic

Authored by etienneb on May 3 2016, 8:15 AM.

Details

Summary

This AST matcher will match a given CastExpr kind.
It's an narrowing matcher on CastExpr.

Diff Detail

etienneb retitled this revision from to Add an AST matcher for CastExpr kind.May 3 2016, 8:15 AM
etienneb updated this object.
etienneb added reviewers: alexfh, sbenza, klimek.
etienneb added a subscriber: cfe-commits.
aaron.ballman added a subscriber: aaron.ballman.

Is this required for some purpose?

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

Also, be sure to regenerate the documentation from dump_ast_matchers.py

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

Also, be sure to regenerate the documentation from dump_ast_matchers.py

Ah, this is the way to generate the doc. Thanks :)

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

Also, be sure to regenerate the documentation from dump_ast_matchers.py

Ah, this is the way to generate the doc. Thanks :)

You're welcome!

sbenza added a comment.May 3 2016, 8:42 AM

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

What we have done in the past with enum-arg matchers is to use string->enum conversion in the dynamic bindings.
See the specialization ArgTypeTraits<attr::Kind> in Marshallers.h.
We could add one for CastKind too.

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

That was my original idea.

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

What we have done in the past with enum-arg matchers is to use string->enum conversion in the dynamic bindings.
See the specialization ArgTypeTraits<attr::Kind> in Marshallers.h.
We could add one for CastKind too.

I think I got the idea, I'll give a try to see the results. Thanks Samuel.

alexfh added a comment.May 3 2016, 8:53 AM

Is this required for some purpose?

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

I agree that when possible, matchers should be available via the dynamic matchers API. It doesn't seem overly complicated to add this support here. As far as I understand, we just need to register the matcher in lib/ASTMatchers/Dynamic/Registry.cpp and add support for CastKind argument type (by adding a corresponding ArgTypeTraits instantiation). Etienne, can you try this?

I agree that when possible, matchers should be available via the dynamic matchers API. It doesn't seem overly complicated to add this support here. As far as I understand, we just need to register the matcher in lib/ASTMatchers/Dynamic/Registry.cpp and add support for CastKind argument type (by adding a corresponding ArgTypeTraits instantiation). Etienne, can you try this?

  1. The parameter is passed as a string (which is not the case for the hasCastKind matcher): hasAttr("attr::CUDADevice").
  1. There is no easy way to list every cast kind. Which means we need to hardcode them (or iterate over the domain) [both solution sounds terrible to me].
static clang::CastKind getCastKind(llvm::StringRef AttrKind) {
  return llvm::StringSwitch<clang::CastKind>(AttrKind)
    .Case("CK_Dependent", CK_Dependent)
   [...]    <<-- list every cast kind here.
    .Default(clang::CastKind(-1));
}

So even if the above solution is working, we still need to call it that way (as a string):
clang-query> match castExpr(hasCastKind("CK_Dependent"))

I agree that when possible, matchers should be available via the dynamic matchers API. It doesn't seem overly complicated to add this support here. As far as I understand, we just need to register the matcher in lib/ASTMatchers/Dynamic/Registry.cpp and add support for CastKind argument type (by adding a corresponding ArgTypeTraits instantiation). Etienne, can you try this?

  1. The parameter is passed as a string (which is not the case for the hasCastKind matcher): hasAttr("attr::CUDADevice").

Correct. The dynamic registry has no support for enumerations of values, and so it uses strings instead. I think that's fine here as well.

  1. There is no easy way to list every cast kind. Which means we need to hardcode them (or iterate over the domain) [both solution sounds terrible to me]. ` static clang::CastKind getCastKind(llvm::StringRef AttrKind) { return llvm::StringSwitch<clang::CastKind>(AttrKind) .Case("CK_Dependent", CK_Dependent) [...] <<-- list every cast kind here. .Default(clang::CastKind(-1)); } `

This is unfortunate, but is likely the way we would move forward.

So even if the above solution is working, we still need to call it that way (as a string):
clang-query> match castExpr(hasCastKind("CK_Dependent"))

Correct, that's expected behavior for clang-query (though I would love if someday we could expose actual enumerations somehow instead of string literals).

though I would love if someday we could expose actual enumerations somehow instead of string literals

I would like too. Ditto for "equals" which is missing :)

etienneb updated this revision to Diff 56022.May 3 2016, 10:35 AM

add dynamic parsing

So even if the above solution is working, we still need to call it that way (as a string):
clang-query> match castExpr(hasCastKind("CK_Dependent"))

Correct, that's expected behavior for clang-query (though I would love if someday we could expose actual enumerations somehow instead of string literals).

It is not hard to do, but it would require changing the parser, the registry, the type-erased value wrapper, etc.
The late conversion from "string" to enum was the easiest solution at the time.

aaron.ballman added inline comments.May 3 2016, 11:18 AM
lib/ASTMatchers/Dynamic/Marshallers.h
102

This might be an awful idea, but let's explore it.

What if we moved the CastKind enumerator names into a .def (or .inc) file and use macros to generate the enumeration as well as this monster switch statement? We do this in other places where it makes sense to do so, such as in Expr.h:

  enum AtomicOp {
#define BUILTIN(ID, TYPE, ATTRS)
#define ATOMIC_BUILTIN(ID, TYPE, ATTRS) AO ## ID,
#include "clang/Basic/Builtins.def"
    // Avoid trailing comma
    BI_First = 0
  };
etienneb added inline comments.May 4 2016, 3:58 PM
lib/ASTMatchers/Dynamic/Marshallers.h
102

Does the dynamic matching is used somewhere else than clang-query?
I wonder the impact of refactoring to support them if it's barely used.
It can't be worse than before as it wasn't supported at all (the matcher didn't exists).

I believe there is a larger cleanup to do to support correctly dynamic matcher like "equals".
And, this case is one among others.

I'm not a fan of this huge switch that may just get out-of-sync with the original enum.

I'm still in favor of adding this matcher to the unsupported list until we push the more complicated fix.
[which may fall in my plate anyway]

Any toughs?

sbenza added inline comments.May 5 2016, 7:50 AM
lib/ASTMatchers/Dynamic/Marshallers.h
102

I'm not a fan of this either.
If the enum has to be used in this way, it should be refactored to be generated with an .inc/.def file.

etienneb updated this revision to Diff 56427.May 6 2016, 10:02 AM

revert to previous solution

etienneb updated this revision to Diff 56428.May 6 2016, 10:04 AM

fix unittests

Get back to this solution.
comments?

sbenza added inline comments.May 10 2016, 2:10 PM
lib/ASTMatchers/Dynamic/Registry.cpp
78

Is it registered or not?
You add this comment, but also add the matcher in the registry below.

etienneb updated this revision to Diff 57044.May 12 2016, 8:52 AM
etienneb marked an inline comment as done.

rebase over AST modifications

etienneb marked 3 inline comments as done.May 12 2016, 8:58 AM

This patch has been modified to support dynamic matchers.
It rely on : http://reviews.llvm.org/D20207

Comments?

lib/ASTMatchers/Dynamic/Marshallers.h
102

moved to that solution. (let try it)

lib/ASTMatchers/Dynamic/Registry.cpp
78

bad revert.
I'm making a patch to allow dynamic matcher, and I'll come back fixing this CL.

etienneb updated this revision to Diff 57101.May 12 2016, 2:20 PM
etienneb marked an inline comment as done.

regenerate doc

Drive-by thought: I think it would be useful to be able to match implicit casts separately from explicit casts when using this new matcher.

etienneb updated this revision to Diff 57123.May 12 2016, 6:22 PM

add unittest

Drive-by thought: I think it would be useful to be able to match implicit casts separately from explicit casts when using this new matcher.

Jonathan, I think what you are asking for is already supported.
I added an unittest to confirm it.

EXPECT_TRUE(matches("char *p = 0;",
            implicitCastExpr(hasCastKind(CK_NullToPointer))));

If I misunderstood your comment, could you provide an example.

etienneb updated this revision to Diff 57124.May 12 2016, 6:26 PM

nit: indentation

Drive-by thought: I think it would be useful to be able to match implicit casts separately from explicit casts when using this new matcher.

Jonathan, I think what you are asking for is already supported.
I added an unittest to confirm it.

EXPECT_TRUE(matches("char *p = 0;",
            implicitCastExpr(hasCastKind(CK_NullToPointer))));

If I misunderstood your comment, could you provide an example.

Oh, right. That /is/ exactly what I meant. Excellent.

aaron.ballman accepted this revision.May 13 2016, 6:53 AM

LGTM! Thank you for this!

This revision is now accepted and ready to land.May 13 2016, 6:53 AM
Prazek added a subscriber: Prazek.May 13 2016, 7:00 AM

+1 I will probably also use this.

Does hasCastKind works for implicitCastExpr?

+1 I will probably also use this.

Does hasCastKind works for implicitCastExpr?

Yes, there is a unittest with the code.

etienneb closed this revision.May 13 2016, 12:42 PM