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