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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

//*un-forgets to actually post comments*//

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

> Cool! I have found this message-semantic idea useful in Unity where every `GameObject` could talk with each other in a very generic way (https://docs.unity3d.com/ScriptReference/GameObject.SendMessage.html).


I mean, for instance, the whole Objective-C is built in this very manner. But it's not always worth it to opt in into such highly dynamic system that bypasses all type checks - in many cases there's a more straightforward solution.

In D58367#1444683 <https://reviews.llvm.org/D58367#1444683>, @Szelethus wrote:

> 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 mean, you hunted down checker names by making sure all objects are constructed properly, which probably means that it's harder to construct these objects improperly for testing purposes. I'm not really sure - maybe it has always been that way :) Anyway, all i need is a `CheckName`, which is merely a string under the hood, but a very hard-to-obtain one.

> 
> 
>> 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.

Yeah, this one's really nice. Another place where i find unittests to be important is when it comes to checking the separation of concerns, eg. the story behind D23963 <https://reviews.llvm.org/D23963> ("`RegionStore` isn't responsible for the semantics of brace initialization"). I'd love to have unit tests for our environment/store/constraint managers and i'll definitely make some when i find myself implementing a new data structure of this kind.


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

https://reviews.llvm.org/D58367