[PATCH] D137346: -Wunsafe-buffer-usage: Initial commit - Transition away from raw buffer accesses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:29
+  /// Invoked when an unsafe operation over raw pointers is found.
+  virtual void handleUnsafeOperation(const Stmt *Operation) = 0;
+};
----------------
xazax.hun wrote:
>  What is the purpose of the handler, would this add the fixit hints? In that case, is this the right abstraction level? E.g., do we want to group these by the declarations so the handler can rewrite the declaration and its uses at once? 
You're absolutely right, we want to group fixits by declarations when fixits are actually available.

But we still need to cover the case when fixits *aren't* available, and in this case it doesn't make sense to group anything, so this interface isn't going away.

And on top of that, there's layers to grouping. For instance, in this code
```
void foo(int *p, size_t size) {
  int *q = p;
  for (size_t i = 0; i < size; ++i)
    q[i] = 0;
}
```
we'll need to attach the fix to the declaration `p` rather than `q`, as we aim to provide a single fixit for these two variables, because otherwise we end up with a low-quality fix that suppresses the warning but doesn't carry the span all the way from the caller to the use site.

Then if we have two parameters that we want to fix simultaneously this way, the fix will have to be inevitably grouped by *function* declaration.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:22
+// Scan the function and return a list of gadgets found with provided kits.
+static GadgetList findGadgets(const Decl *D) {
+
----------------
xazax.hun wrote:
> Is this not a FunctionDecl because of ObjCMethod? At some point I wonder if we should have an abstraction that works for both. Would NamedDecl work?
Yupp.

Well, we do have https://clang.llvm.org/doxygen/classclang_1_1AnyCall.html But it's not like the code actually cares at this point. And once it does (eg. for the purposes of fixits), it'll have to consider them separately anyway.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:35-36
+
+  GadgetList G;
+  MatchFinder M;
+
----------------
xazax.hun wrote:
> Move these close to their uses?
The code in between is actually instantly deleted by the follow-up patch :)

(D137348)


Repository:
  rC Clang

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

https://reviews.llvm.org/D137346