aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM
================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:34
+// through the handler class.
+void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
+
----------------
NoQ wrote:
> 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.
Hmmm, I may regret this later (I often do when suggesting laxity with `const`),
but I suppose it's reasonable to leave the interface mutable. I usually prefer
things to be `const` up front and then relax the restriction later once we have
a need, but this is a case where relaxing that later would be about as viral as
adding `const` later.
================
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.
----------------
NoQ wrote:
> 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?
> Hmm, is there a way to write the .td file entry so that the source range
> becomes mandatory?
Not to my knowledge; we could maybe thread through enough information to assert
if the diagnostic is called without a source range, but I'm not certain we
could reasonably get a compile-time error if the range isn't provided.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2152-2154
+ void handleUnsafeOperation(const Stmt *Operation) override {
+ S.Diag(Operation->getBeginLoc(), diag::warn_unsafe_buffer_usage);
+ }
----------------
NoQ wrote:
> 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];
> ~
> ```
> Operation->getSourceRange() does the trick right?
Yup!
> Or do you think it'd be better to do it like this?
I prefer the first form where the whole operation is highlighted instead of
just an operand of the operation. I think that'll make more sense for
situations like:
```
a + i;
```
to highlight the whole expression instead of just `a`. It also helpfully means
I don't have to ask for a test like: `i[a]` and see if we highlight the correct
"raw buffer" operand. :-D
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
