[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Csaba Dabis via Phabricator via cfe-commits]

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