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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In general I think it would be cool to think all of the problems in the same time as these really depends on the `CoreEngine` and how we operates with `ExplodedNodes`.

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

> In particular, the implementation you propose does a lot more than adding a note: it also adds, well, a transition.


I think a possible workaround to handle special nodes is adding them in `CoreEngine` as successors (instead of predecessors) so when we are traversing backwards it immediately known the next (pred) "not-special" node will be its parent. This is the same logic in `ConditionBRVisitor` where we only work with pair of nodes to see if we would report something.

> The issue here is that the checker doesn't always have an ability to mutate the graph by adding nodes. Now it becomes more of a problem because the checker may want to emit notes more often than it wants to generate nodes. This problem can be easily mitigated by passing a different sort of context (not `CheckerContext` but something else) into these callbacks that will store lambdas in a different manner.
>  I think this is not a big flexibility issue on its own because it should be possible to make lambdas from multiple sources cooperate with each other by sharing a certain amount of arbitrary state information within the BugReport object.

I think it ties with the problem above, so having some special `ExplodedNode` could fix both of them because may you would introduce some `ExplodedNodeContext` to store more information.

> In D58367#1404722 <https://reviews.llvm.org/D58367#1404722>, @Charusso wrote:
> 
>>   const NoteTag *setNoteTag(StringRef Message) {
>>     return Eng.getNoteTags().setNoteTag(Message);
>>   }
>>   
> 
> 
> Mmm, i don't think i understand this part. Like, `getNoteTag()` is not really a getter. It's a factory method on an allocator that causes it to produce an object. What is the semantics of a setter that would correspond to it?

All the problems what you mentioned is not really a job of a factory, so I just emphasized we only get the true benefit of the callback in the get part. I think leave it as it is now makes sense but later on there would be a lot more function to hook crazy lambdas to *set* information.

Please note that it is a very high-level feedback, as I just checked the `CoreEngine` and saw why we are splitting states.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58367