[PATCH] D137348: [-Wunsafe-buffer-usage] Introduce an abstraction for fixable code patterns.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:40
+
+  /// Determine if a kind is a safe kind. Slower than calling isSafe().
+  static bool isSafeKind(Kind K) {
----------------
We could have a `GADGET_RANGE(UnsafeGadgets, Increment, Decrement)`, which could expand to `BEGIN_##x = Increment, END_##x = Decrement,` when declaring the `Kind` enum, similarly how `SymExpr::Kind::Begin_BINARYSYMEXPRS` is defined.
That way this code could look like:
```lang=C++
return !(Kind::Begin_UnsafeGadgets <= K && K <= Kind::End_UnsafeGadgets);
```


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:43-45
+#define UNSAFE_GADGET(x)                                                       \
+    case Kind::x:
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
----------------
I'd advocate for using the same macro variable name here as an XMACRO argument as it's present in the definition as parameter.
Same for the rest of the XMACRO expansions.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:87
+  static bool classof(const Gadget *G) { return !isSafeKind(G->getKind()); }
+  bool isSafe() const override { return false; }
+};
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:102
+  static bool classof(const Gadget *G) { return isSafeKind(G->getKind()); }
+  bool isSafe() const override { return true; }
+};
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:123
+      hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))
+    ).bind("op"));
+  }
----------------
I thought this matcher was already bound as `"Increment"` by `x ## Gadget::matcher().bind(#x)` inside `GadgetFinderCallback::run()`


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:150
+
+  const Stmt *getBaseStmt() const override { return Op; }
+};
----------------
I'd advocate for covariant return types for such cases.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:163
 
     void run(const MatchFinder::MatchResult &Result) override {
+      // Figure out which matcher we've found, and call the appropriate
----------------
I wonder if we should assert that we only expect exactly one match (entry) inside `Result.Nodes`.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:190-191
+#undef GADGET
+        // FIXME: Is there a better way to avoid hanging comma?
+        unless(stmt())
+      ))
----------------
I was thinking of constructing a `std::vector<DynTypedMatcher>` of the matchers of the disjunction because the extra comma is allowed inside the initializer expression.
After that, you could probably use `constructVariadic(VO_AnyOf, ...)` as demonstrated by the `TEST(ConstructVariadic, MismatchedTypes_Regression)` unit-test. But TBH, I don't think it's more readable than this :D


Repository:
  rC Clang

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

https://reviews.llvm.org/D137348