llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.81k stars 11.91k forks source link

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

Open steveire opened 6 years ago

steveire commented 6 years ago
Bugzilla Link 38318
Version trunk
OS Linux

Extended Description

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.

steveire 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.

llvmbot 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?