aaron.ballman added inline comments.

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:4821
@@ +4820,3 @@
+///   initializer for i.
+AST_MATCHER(Expr, nullPointerConstant) {
+  return Matcher<Expr>(
----------------
sbenza wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > sbenza wrote:
> > > > sbenza wrote:
> > > > > Use AST_MATCHER_FUNCTION instead, where the return value is the 
> > > > > matcher (instead of the application of the matcher).
> > > > > It is simpler to write and since it has no arguments it will memoize 
> > > > > the matcher and construct it only once.
> > > > Maybe use Expr::isNullPointerConstant?
> > > Ah, interesting! I hadn't known about that macro. Thank you.
> > Oh, hey, that's easier still! Would I still use AST_MATCHER_FUNCTION in 
> > that case though, or should that remain a simple AST_MATCHER?
> One or the other.
> The _FUNCTION macro is for when the matcher itself is just a matcher 
> expression. Saves typing and CPU.
> Generating the temporary matchers on each match is expensive as they do 
> memory allocation.
> If you use Expr::isNullPointerConstant you won't be making any matchers.
> 
One interesting problem I ran into with using Expr::isNullPointerConstant is 
that it makes the matcher overly aggressive with nullptr. Given:
```
AST_MATCHER(Expr, nullPointerConstant) {
  QualType QT = Node.getType();
  if (QT->isNullPtrType())
    return true;

  return QT->isPointerType() &&
         Node.isNullPointerConstant(Finder->getASTContext(),
                                    Expr::NPC_NeverValueDependent);
}
```
If you attempt to match `somePtr = nullptr;` with 
`expr(nullPointerConstant())`, you will get two matches. One for the 
initializer expression of the declaration, and one for the implicit cast 
expression. However, with `somePtr = __null` or `somePtr = 0`, you only get a 
single match (for the initializer expression). This seems like surprising 
behavior, but I'm not quite certain of the best way to address it, if it should 
be addressed at all.


http://reviews.llvm.org/D17034



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to