NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+ switch (SmartPtrMethod) {
+ case Constructor: {
----------------
NoQ wrote:
> 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";
> }));
> ```
(forgot `, llvm::raw_ostream OS` in the last snippet; probably forgot a bunch
of other stuff because i didn't try to actually compile these snippets)
================
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());
----------------
NoQ wrote:
> 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 "";
> ```
(i strongly recommend having test cases for both of these issues)
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
