Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

ast_matchers::findAll does not work with elements of different type to root #37291

Open Quuxplusone opened 6 years ago

Quuxplusone commented 6 years ago
Bugzilla Link PR38318
Status NEW
Importance P enhancement
Reported by Stephen Kelly (steveire@gmail.com)
Reported on 2018-07-25 14:54:49 -0700
Last modified on 2018-08-01 09:28:19 -0700
Version trunk
Hardware PC Linux
CC klimek@google.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
We should be able to do something like

 Decl* someRootDeclContext = ...;

 clang::ast_matchers::match(
   decl(findAll(callExpr()))
   , *someRootDeclContext, *Result.Context);

However, that does not work because findAll is implemented with eachOf and

   decl(eachOf(callExpr(), expr()))

doesn't work for the same reason

   decl(callExpr())

doesn't work - a decl is never a callExpr.

findAll should be changed to account for this use-case.
Quuxplusone commented 6 years ago

Given that findAll is literally just convenience for eachOf(Matcher, forEachDescendant(Matcher)), can't you use forEachDescendant() directly, as the expr matcher can never match a decl anyway?

Quuxplusone commented 6 years ago
Manuel - the implementation of findAll is an implementation detail. No user
should have to know it.

The documentation of findAll says "Matches if the node or any descendant
matches".

Either that description is too naive and the documentation should be updated,
or the implementation could be changed to match the existing documentation. I
think the latter would be more useful to users.

The ast_matchers::match documentation specifically recommends findAll:

/// If you want to find all matches on the sub-tree rooted at \c Node (rather
/// than only the matches on \c Node itself), surround the \c Matcher with a
/// \c findAll().

Even though the implementation under discussion here of findAll makes it
unsuitable for that.

I am aware that in this case, forEachDescendant() can be used, as I wrote here:

 http://lists.llvm.org/pipermail/cfe-dev/2018-July/058625.html

You could see this as a note that the documentation of
clang::ast_matchers::match should be better, if you wish.