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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:63
+
+  Gadget(Kind K) : K(K) {}
+
----------------
aaron.ballman wrote:
> Should this be an explicit constructor? (Same question applies below to derived classes as well.)
What's the value of making it `explicit` given that it's an abstract class that can't be constructed directly anyway?


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:113
+      : UnsafeGadget(Kind::Increment),
+        Op(Result.Nodes.getNodeAs<UnaryOperator>("op")) {}
+
----------------
aaron.ballman wrote:
> Should we make a `static constexpr const char *` for these strings so we can give it a name and ensure it's used consistently?
I think it makes perfect sense to have different bind-names in different classes. They don't all correspond to the same role the bound expression plays.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:123
+      hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))
+    ).bind("op"));
+  }
----------------
steakhal wrote:
> I thought this matcher was already bound as `"Increment"` by `x ## Gadget::matcher().bind(#x)` inside `GadgetFinderCallback::run()`
Yeah it's a bit redundant but it makes the class properly incapsulated, not reliant on the external machine to use it in a certain way. A bit more resistant to renaming as well.

And while //this time// these two bindings coincided, in the general case they can be different. So I wanted to establish an idiom that works in all cases.

If we ever run into performance problems, I'm totally open to optimizing this.


================
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
----------------
steakhal wrote:
> I wonder if we should assert that we only expect exactly one match (entry) inside `Result.Nodes`.
You mean like, exactly one if-statement succeeds? That'd require us to run the entire list every time (at least in debug mode) but it's probably not too bad. I'll look for a clean solution^^


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:190-191
+#undef GADGET
+        // FIXME: Is there a better way to avoid hanging comma?
+        unless(stmt())
+      ))
----------------
steakhal wrote:
> 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
One way or another, D138253 cleans up this FIXME.


Repository:
  rC Clang

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

https://reviews.llvm.org/D137348