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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:149-152
+// Matches a `UnaryOperator` whose operator is pre-increment:
+AST_MATCHER(UnaryOperator, isPreInc) {
+  return Node.getOpcode() == UnaryOperator::Opcode::UO_PreInc;
+}
----------------
Oh interesting, so `hasOperatorName` wasn't sufficient, because operators `++` and `++` have the same "name"?

Yeah, sounds like a universally useful matcher, we should commit it to `ASTMatchers.h` for everybody to use.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:663
+    return stmt(isInUnspecifiedPointerContext(expr(
+        ignoringImpCasts(unaryOperator(isPreInc()).bind(UPCPreIncrementTag)))));
+  }
----------------
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.


================
Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp:25
+
+  // Don't know how to fix the following cases:
+  // CHECK-NOT: fix-it:"{{.*}}":{
----------------
It's probably a good idea to explicitly say `FIXME:` if we think these cases should eventually be fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144304/new/

https://reviews.llvm.org/D144304