NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:483
+ llvm::raw_ostream &OS) {
+ BR.markInteresting(ThisRegion);
+ OS << "Smart pointer";
----------------
Wait, why are we marking the region interesting here? Not every null smart
pointer is interesting, only the ones that are actually dereferenced.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:497-499
+ OS << "Smart pointer";
+ checkAndPrettyPrintRegion(OS, ThisRegion);
+ OS << " is non-null";
----------------
Also i'm starting to suspect that these notes aren't actually needed when
there's no "Assuming..."; in particular, we don't emit such notes for raw
pointers, so we probably shouldn't emit them for smart pointers either. There's
anyway going to be a note about "Taking true branch..." or something like that,
which is sufficient to understand what was the smart pointer.
I.e., this note is unnecessary:
```lang=c++
void foo() {
std::unique_ptr<int> P = nullptr;
if (P) { // Smart pointer 'P' is null
// Taking false branch
}
}
```
The second note, "Taking false branch", is sufficient for explaining to the
user that the smart pointer is known to be null. The user may ask why the smart
pointer is null, but the note isn't explaining it. The way you marked the
region as interesting (as i noticed in the above inline comment) would have
indeed explained why it's null, but at this point we draw the line and believe
that if the region isn't already interesting then the user most likely doesn't
need to know why it's null (and if it's already interesting then there's no
need to mark it again).
But this note is necessary:
```lang=c++
void foo(std::unique_ptr<int> P) {
if (P) { // Assuming smart pointer 'P' is null
// Taking false branch
}
}
```
This note conveys the extra information that we don't know for a fact that the
smart pointer is null on the current path based on previous analysis, but
instead the analyzer is *assuming* that it's possible that it's null due to the
presence of the check in the code. That's a big deal.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86027/new/
https://reviews.llvm.org/D86027
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
