[cfe-commits] [PATCH] New matcher for MaterializeTemporaryExpression.

[Sean Silva]

  It seems like it would almost make more sense to move these great doc comments with great examples of source constructs that the AST node corresponds to into the documentation for the AST nodes themselves, and then have single header (say, "StmtMatchers.h") with a block comment at the top saying something to the tune of

  "each `Stmt` subclass has a corresponding matcher here, with a name which is the name of the AST node with the first letter lowercase rather than upper, so for example `MaterializeTemporaryExpr`'s corresponding matcher is `materializeTemporaryExpr`. If you are unsure about what a given `Stmt` subclass corresponds to at the source level, each corresponding `Stmt` subclass's doxygen comment contains a number of examples which should help you get a feel for what the AST node corresponds to at the source level."

  And then you could just stamp out all of them. I'm not sure how you're determining when to add exact AST node matchers, but I can't see a compelling reason to not just add all of them at once (well, besides the ever-present lack of time :). Of course, all of what I said for `Stmt`'s could just as easily be said for `Decl`'s or other AST nodes.

  Maybe you could make it even more uniform from an API point of view and have a matcher `astNode<T>` where `T` is any AST node and which matches that node exactly, so the `materializeTemporaryExpr` you have in this patch could be written as `astNode<MaterializeTemporaryExpr>`. I think that from an API point of view this is really clean, since then the user only has to learn one matcher in order to match any exact AST node they want (this also seems like a huge win from a documentation perspective as well, since you now only need to document one matcher in order to cover all exact AST node matchers).

  I understand that the `astNode<T>` is a bit more verbose, but the clarity and API transparency that it affords is I think //much// more important, since you can recover the syntax sugar trivially with aliases. For example, suppose that I just want to match records; clang has an AST node for that, which is `RecordDecl`, so I know I just have to do `astNode<RecordDecl>` and that's it. However, with the current design, I need to wade through docs to find it (it doesn't seem to exist after wading and grepping), or worse be deceived and use `record()`, which doesn't match `RecordDecl`'s, but actually `CXXRecordDecl`'s!

  Is there something that I'm missing that complicates doing it that way? Could you compare/contrast your current approach with the one I just described?

  In a sense, I guess what my radar is picking up on here is that you have some "boilerplate" AST matchers here which correspond exactly with AST nodes, while other ones don't, like allOf(), yet this distinction doesn't appear to be being called out in documentation or well-factored within the code itself (or documentation). Also, it seems like a documentation Single Point Of Truth violation to have these great examples of what the AST node corresponds to here and not on the actual AST node---these examples would probably also be of interest to a person reading documentation of the AST node. Having the examples here makes perfect sense for matchers which are not matching exact AST nodes, though.

  Thanks for bearing with this novel.

http://llvm-reviews.chandlerc.com/D20