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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: njames93, sammccall, LegalizeAdulthood, klimek.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:34
+// through the handler class.
+void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
+
----------------
Do we need the interface to accept a non-const reference?


================
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.
----------------
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.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:50
+  M.addMatcher(
+      stmt(forEachDescendant(
+        stmt(
----------------
Errr, this looks expensive to me, but maybe I'm forgetting (I can't recall if it's ancestor or descendant that's more expensive, I think it's ancestor though). @njames93 @LegalizeAdulthood @klimek @sammccall -- do you have concerns here or know of a better approach?


================
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
----------------
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.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2152-2154
+  void handleUnsafeOperation(const Stmt *Operation) override {
+    S.Diag(Operation->getBeginLoc(), diag::warn_unsafe_buffer_usage);
+  }
----------------
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.


================
Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:1
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+
----------------
No need to pin to a specific language mode.


Repository:
  rC Clang

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

https://reviews.llvm.org/D137346