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

Wed Mar 27 09:52:21 PDT 2019

Szelethus accepted this revision.
Szelethus added a comment.

Amazing work! Thanks!

In D58367#1443783 <https://reviews.llvm.org/D58367#1443783>, @NoQ wrote:

> Remove memoization for now. Address a few inline comments. I'm mostly done with this first patch, did i accidentally miss anything?
>
> In D58367#1402796 <https://reviews.llvm.org/D58367#1402796>, @Szelethus wrote:
>
> > 1. It would be great to have unit tests for this.
>
>
> Mm, i have problems now that checker registry is super locked down: i need a bug report object => i need a checker (or at least a `CheckName`) => i need the whole `AnalysisConsumer` => i can no longer override `ASTConsumer` methods for testing purposes (because `AnalysisConsumer` is a final sub-class of `ASTConsumer`). Do you have a plan B for that?

Saw this, will think about it! Though I'm a little unsure what you mean under the checker registry being locked down.

> I also generally don't see many good unit tests we could write here, that would add much to the integration tests we already have, but this memoization problem could have made a nice unit test.

I don't insist, but unlike most of the subprojects within clang, we have very-very few infrastructural unit tests. I don't even find them to be that important for testing purposes, but more as minimal examples: I remember that `unittests/StaticAnalyzer/RegisterCustomChecker.cpp` was a godsent when I worked on checker dependencies.

================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:600
+public:
+  typedef std::function<std::string(BugReporterContext &, BugReport &)>
+      Callback;
----------------
Prefer using.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58367/new/

https://reviews.llvm.org/D58367