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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 192372.
NoQ marked 6 inline comments as done.
NoQ added a comment.

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?

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.

In D58367#1423497 <https://reviews.llvm.org/D58367#1423497>, @Charusso wrote:

> In D58367#1423451 <https://reviews.llvm.org/D58367#1423451>, @NoQ wrote:
>
> > Found a bug! The lambda has to manually make sure that the bug report actually does have something to do with the checker.
>
>
> I think message-semantic is better than storing some data in two places. BugReport has `getCheckName()` and may if the `NoteTag` knows the name of its author then it is comparable.
>  I am not sure but may you could hack the lifetime of that `StringRef` from `CheckName` to not to report on something purged out (/not exists?).


Storing and comparing the checker pointer ought to be cheaper and more precise than storing and comparing its name as a string that can be easily extracted from it. Still, storing a string may be interesting when it comes to identifying non-checker note sources. I guess we'll have to think more about that.


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

https://reviews.llvm.org/D58367

Files:
  clang/include/clang/Analysis/ProgramPoint.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/mig.mm

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58367.192372.patch
Type: text/x-patch
Size: 12238 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190326/ade59eec/attachment-0001.bin>