[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;
+};
----------------
NoQ wrote:
> 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.
I'll add comments in D138253 to clarify the separation of concerns more clearly, once the methods [between which concerns are separated] actually get defined.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2152-2154
+  void handleUnsafeOperation(const Stmt *Operation) override {
+    S.Diag(Operation->getBeginLoc(), diag::warn_unsafe_buffer_usage);
+  }
----------------
aaron.ballman wrote:
> I think this interface needs an additional `SourceRange` parameter that can be passed in as a streaming argument, along these lines. This way you'll get squiggles for the problematic part of the expression.
`Operation->getSourceRange()` does the trick right? Like this:
```
warning: unchecked operation on raw buffer in expression

  a[i];
  ~~~~
```
I suspect that the Operation is forever going to be the exact thing we want to highlight, there's virtually no reason for it to be anything else.

Or do you think it'd be better to do it like this?
```
warning: unchecked operation on raw buffer in expression

  a[i];
  ~
```


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

https://reviews.llvm.org/D137346