ziqingluo-90 added inline comments.
================ Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:663 + return stmt(isInUnspecifiedPointerContext(expr( + ignoringImpCasts(unaryOperator(isPreInc()).bind(UPCPreIncrementTag))))); + } ---------------- NoQ wrote: > You're not checking whether the operand is a variable. This isn't incorrect, > given that the fixable will be thrown away if it doesn't claim any variables. > However, for performance it makes sense to never match things that don't act > on variables, so that to never construct the fixable object in the first > place. The check for the variable being an actual `VarDecl` is also useful > and can be made here instead of the `getFixit` method. > > I think this is something we should do consistently: //if it's not the right > code, stop doing things with it as early as possible//. Premature > optimizations are evil, but in these cases it's not much of an optimization > really, it's just easier to write code this way from the start. > > It also makes the code easier to read: you can easily see which patterns the > gadget covers, by just reading the `matcher()` method. aha, I agree with you and I like your last sentence the most! Treating the `matcher()` methods as documents, i.e., we are guaranteed that a matched node conforms to the "description" of the `matcher()`. So that we can get rid of defensive checks. For example, the `if (auto DRE = dyn_cast<DeclRefExpr>(Node->getSubExpr()))` statement at line 671 could be a defensive check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144304/new/ https://reviews.llvm.org/D144304 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
