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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def:1
+//=- UnsafeBufferUsageGadgets.def - List of ways to use a buffer --*- C++ -*-=//
+//
----------------
NoQ wrote:
> aaron.ballman wrote:
> > Is this file named properly if it is also going to handle `SAFE_GADGET`?
> UnsafeBufferUsage is the name of the analysis. It's somewhat valuable to keep this file next to `UnsafeBufferUsage.h` so that it was obvious at a glance that these two files work together.
> 
> I'm open to renaming the entire analysis though, a non-judgemental "BufferUsage analysis" totally works for me, WDYT?
> 
> Or I can make a folder. But I don't expect more than 2 files in this folder.
I expect @malavikasamak to do some renaming in a follow-up patch in the near future as she seeks to introduce more precise distinction between unsafe and safe gadgets.


================
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) {
----------------
steakhal wrote:
> 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);
> ```
Nuked this entire facility. It's always possible to simply call `isSafe()`.


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


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:107
+/// out of bounds.
+class IncrementGadget : public UnsafeGadget {
+  const UnaryOperator *Op;
----------------
NoQ wrote:
> xazax.hun wrote:
> > How deep will the `Gadget` Hierarchy be? Using inheritance only to classify safe and unsafe gadgets feels like a very heavy weight solution to a relatively simple problem.
> I'm expecting `UnsafeGadget` and `SafeGadget` to grow some common methods in the future. But I'm already second-guessing this decision (https://reviews.llvm.org/D137379?id=473092#inline-1340017) so I guess I'll ungrow these intermediate classes if things become too tedious.
To answer the original question, `SafeGadget` and `UnsafeGadget` are most likely going to be the only intermediate class. So, it'll be 2 abstract classes deep.


================
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
----------------
NoQ wrote:
> 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^^
Ok I added this huge `#ifdef NDEBUG` thing. I agree that it's valuable but I'm also somewhat worried that it may diverge from the non-debug code.


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

https://reviews.llvm.org/D137348