[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment

[Ziqing Luo via Phabricator via cfe-commits]

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