[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/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def:1
+//=- UnsafeBufferUsageGadgets.def - List of ways to use a buffer --*- C++ -*-=//
+//
----------------
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.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:17-21
+static auto hasPointerType() {
+  return anyOf(hasType(pointerType()),
+               hasType(autoType(hasDeducedType(
+                   hasUnqualifiedDesugaredType(pointerType())))));
 }
----------------
aaron.ballman wrote:
> Something to think about: ObjC pointers are pointer-like but not actual pointers; should those be covered as well (via `isAnyPointerType()`)?
Great question! Technically unsafe operations on ObjC pointers are not a compile error, but they never actually make sense because you can't have buffers of ObjC objects. Technically, you're not even supposed to know their size or layout until runtime (https://en.wikipedia.org/wiki/Objective-C#Non-fragile_instance_variables). So this is an insanely exotic situation that we most likely won't be covering.


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


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:113
+      : UnsafeGadget(Kind::Increment),
+        Op(Result.Nodes.getNodeAs<UnaryOperator>("op")) {}
+
----------------
aaron.ballman wrote:
> NoQ wrote:
> > 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.
> Sorry, I was unclear. I meant a private data member of `IncrementGadget` so that the constructor and the `matcher()` functions use a named constant rather than a string literal.
Oh, fair enough!


Repository:
  rC Clang

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

https://reviews.llvm.org/D137348