Charusso requested changes to this revision.
Charusso added a comment.
This revision now requires changes to proceed.
First of all, thanks you for working on this, as I wanted to do the same, but I
did not know how to. I did not know also that 15 of the checkers already
implements `BugReporterVisitors`, but I completely shocked it took 10 years of
pain to rewrite it. It feels like this patch a little-bit brute-force, and I
believe this should be the future direction of reporting. Also I believe in
that we could hook a lot more useful information from *all* the checkers, and
we do not care that much about the core `BugReporterVisitors` as they do their
job enough well. So with that, I would out-chain this patch from the
MIG-patches because it is a lot more serious problem-set. I also would like to
ask you to take it more serious, as all your mentioned benefits will rely on
this simple and cool approach, so it has to be flexible as possible. I am not
into the internal stuff that much, so I cannot argue about the lack of
`CheckerContext` functions, but I think this is a huge problem and breaks the
flexibility a lot.
I tried out the patch and saw the very recommended source code of
`MallocChecker`'s so lightweight `BugReporterVisitor` implementation and I
think your API is too strict about having a lambda function, so here is my
ideas:
- `CheckerContext.h`: As I see we only work with a `string`, nothing else, so I
would extend that class:
ExplodedNode *addNote(ProgramStateRef State, StringRef Message) {
return addTransitionImpl(State, false, nullptr, setNoteTag(Message)); //
Note the 'setNoteTag()'
}
`getNoteTag()`: you could differentiate a setter with the true getter what you
only use in `TagVisitor::VisitNode()` (where you truly hook your `Callback` to
get extra information later on):
const NoteTag *setNoteTag(StringRef Message) {
return Eng.getNoteTags().setNoteTag(Message);
}
- `BugReporterVisitors.h`: basically a `NoteTag` is a string, nothing else, I
think you would like to hook the `Callback` only when you are about to obtain
the message:
class NoteTag {
std::string Msg;
NoteTag(StringRef Message) : ProgramPointTag(&Kind), Msg(Message)
class Factory {
const NoteTag *getNoteTag(Callback &&Cb)
const NoteTag *setNoteTag(StringRef Message)
};
};
- Documentation: if we are about the rewrite the entire bug-reporting business,
it worth some long doxygen comments.
The outcome is that cool I have to be the reviewer in two patches, and I hope
it helps you creating a better API, even if I got something wrong.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:364
+
+ Optional<std::string> getNote(BugReporterContext &BRC, BugReport &R) const {
+ std::string Note = Cb(BRC, R);
----------------
It feels like it returns the `NoteTag`, so I would rename it as `getMessage()`.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:223
+ /// Produce a program point tag that displays an additional path note
+ /// to the user. This is a lightweirght alternative to the
+ /// BugReporterVisitor mechanism: instead of visiting the bug report
----------------
It is very randomly spaced, `lightweight`.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58367/new/
https://reviews.llvm.org/D58367
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
