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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
