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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
