NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:86
+ if (const auto *DR = dyn_cast<DeclRegion>(DerefRegion)) {
+ auto SmartPtrName = DR->getDecl()->getName();
+ OS << " '" << SmartPtrName << "'";
----------------
Please `getNameAsString()` because `getName()` crashes on anonymous
declarations (eg., lambda captures, anonymous nested structs/unions, etc.).
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96
+
+ const auto *RecordDecl = MethodDecl->getParent();
+ if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) {
+ OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'";
----------------
Aha, this time you checked for anonymous declarations! Good.
That said, i'm not sure type is actually useful for this warning, because
they're roughly all the same.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+ switch (SmartPtrMethod) {
+ case Constructor: {
----------------
I feel this is a bit over-engineered. There's no need to create an enum and
later convert it into a string when you can capture a string directly. Like,
this entire "details" structure of your, it should be just captures instead,
and your strings would make it immediately obvious what kind of note is emitted:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return "";
return Twine("Default constructed smart pointer '") + getSmartPtrName(R) + "'
is null";
}));
```
Hmm, maybe we should also provide an overload with lambdas of
signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that to
make the same one-liners possible with streams? Something like this:
```lang=c++
class CheckerContext {
// ...
NoteTag *getNoteTag(std::function<void(PathSensitiveBugReport &BR,
llvm::raw_ostream OS)> f) {
return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
llvm::SmallString<128> Str;
llvm::raw_svector_ostream OS(Str);
f(BR, OS);
return OS.str();
});
}
// ...
};
```
Then you could do:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return;
OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' is
null";
}));
```
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+ SmallString<128> Msg;
+ llvm::raw_svector_ostream Out(Msg);
+ TagDetails.trackValidExpr(BR);
+ TagDetails.explainSmartPtrAction(Out);
+ return std::string(Out.str());
----------------
Ok, note that note tags are attached to nodes independently of bug reports;
when the report is thrown, only then we know what are the smart pointers that
should be explained.
So there are two checks that you should do here:
1. Check that the bug report is emitted by your checker (eg., by comparing bug
types). If not, don't add notes.
2. Check that the region about which the note speaks is related to your report
(i.e., it's not a completely unrelated smart pointer). You can do that by
marking the smart pointer as "interesting" (i.e.,
`PathSensitiveBugReport::markIntersting()`) when you emit the report, and then
in the lambda you check whether the smart pointer is interesting before you
emit a note. Additionally, you can carry over interestingness when smart
pointers are copied.
This is what i was trying to accomplish with this code snippet that i included
in the examples in the other comment:
```lang=c++
if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return "";
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84600/new/
https://reviews.llvm.org/D84600
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
