[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:34
+// through the handler class.
+void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
+
----------------
aaron.ballman wrote:
> Do we need the interface to accept a non-const reference?
It's same as asking whether the `handle...` methods should be const. Roughly similar to whether `MatchFinder::MatchCallback::run()` should be const. I don't see a reason why not, but also "Why say lot word when few word do trick", could have been a lambda.

As an opposite example, static analyzer's `Checker::check...` callbacks really needed to be const, because carrying mutable state in the checker is often *tempting* but always *terrible*, in a way that's not immediately obvious to beginners.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11731-11732
+// Unsafe buffer usage diagnostics.
+def warn_unsafe_buffer_usage : Warning<"unchecked operation on raw buffer in expression">,
+  InGroup<DiagGroup<"unsafe-buffer-usage">>, DefaultIgnore;
 } // end of sema component.
----------------
aaron.ballman wrote:
> The diagnostic wording isn't wrong, but I am a bit worried about complex expressions. Consider something like `void func(a, b, c + d, e++, f(&g));` -- if you got this warning on that line of code, how likely would you be to spot what caused the diagnostic? I think we need to be sure that issuing this warning *always* passes in an extra `SourceRange` for the part of the expression that's caused the issue so users will at least get some underlined squiggles to help them out.
Yeah, I agree. Later we'll additionally specialize this warning to be more specific than "expression" (eg. "pointer arithmetic", "array access", etc.).

Hmm, is there a way to write the .td file entry so that the source range becomes mandatory?


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:57
+          )
+          // FIXME: Idiomatically there should be a forCallable(equalsNode(D))
+          // here, to make sure that the statement actually belongs to the
----------------
aaron.ballman wrote:
> Heh, this is the sort of thing I was worried about, especially when you consider things like lambda bodies or class definitions with member functions being declared in a function, etc.
Yes, so @ziqingluo-90 is trying to combat this problem in D138329. His solution is non-expensive (single-pass through the AST without any backreference lookups) and I hope it can be standardized into a general-purpose matcher so that we can get rid of the `forCallable()` idiom entirely, even in existing clang-tidy checks, to win some performance and make the code less convoluted.


Repository:
  rC Clang

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

https://reviews.llvm.org/D137346