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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def:1
+//=- UnsafeBufferUsageGadgets.def - List of ways to use a buffer --*- C++ -*-=//
+//
----------------
Is this file named properly if it is also going to handle `SAFE_GADGET`?


================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def:9
+
+#ifndef GADGET
+#define GADGET(name)
----------------
You have some comments below about what gadgets are and what makes one safe or unsafe; should those comments be hoisted into this .def file?


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:17-21
+static auto hasPointerType() {
+  return anyOf(hasType(pointerType()),
+               hasType(autoType(hasDeducedType(
+                   hasUnqualifiedDesugaredType(pointerType())))));
 }
----------------
Something to think about: ObjC pointers are pointer-like but not actual pointers; should those be covered as well (via `isAnyPointerType()`)?


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:37
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+#undef GADGETS
+  };
----------------
NoQ wrote:
> Whoops typo!
We don't even need the `#undef` because the .def file already does that for us. Same observation applies below.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:63
+
+  Gadget(Kind K) : K(K) {}
+
----------------
Should this be an explicit constructor? (Same question applies below to derived classes as well.)


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:113
+      : UnsafeGadget(Kind::Increment),
+        Op(Result.Nodes.getNodeAs<UnaryOperator>("op")) {}
+
----------------
Should we make a `static constexpr const char *` for these strings so we can give it a name and ensure it's used consistently?


Repository:
  rC Clang

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

https://reviews.llvm.org/D137348