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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a reviewer: Charusso.
NoQ marked an inline comment as done.
NoQ added a subscriber: Charusso.
NoQ added a comment.

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

> 2. I guess most of the visitors could be changed to this format, do you have plans to convert them? I'll happily land a hand, because it sounds like a big chore. I guess that would also test this implementation fairly well.


I don't have an immediate plan but i'll definitely convert visitors when i touch them and get annoyed. Also i believe that this new functionality is much more useful for //core// visitors than for checker visitors, simply because there's much more information to reverse-engineer in every visitor. Eg., `trackExpressionValue` would have been so much easier if it didn't have to figure out where did an assignment happen, but instead relied on `ExprEngine` to write down this sort of info as a tag in, say, `evalStore`. There are just too many ways to introduce a store/environment binding that represents moving a value from one place to another and it hurts me when i see all of them duplicated in the visitor as a military-grade portion of spaghetti. The same apples to the `ConditionBRVisitor` that might probably even benefit from having `ConstraintManager` explain the high-level assumption that's being made //as// it's being made; it might have also made @Charusso's work on supporting various sorts of assumptions much easier. At the same time, there aren't that many ways to close a file descriptor, so these are much easier to catch and explain in a visitor, so the main problem in checkers is the boilerplate. Checker APIs, however, are much more important to get polished because they're used much more often.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:222-236
+  /// Produce a program point tag that displays an additional path note
+  /// to the user. This is a lightweirght alternative to the
+  /// BugReporterVisitor mechanism: instead of visiting the bug report
+  /// node-by-node to restore the sequence of events that led to discovering
+  /// a bug, you can add notes as you add your transitions.
+  const NoteTag *getNoteTag(NoteTag::Callback &&Cb) {
+    return Eng.getNoteTags().getNoteTag(std::move(Cb));
----------------
Note: you can't use this API in checker callbacks that don't provide a `CheckerContext`. Let's have a quick look at the list of such callbacks:
- `checkEndAnalysis()`. I'm not super worried about this one because it's a really weird place to put an //intermediate// note. If someone really really needs this, it should be possible to access `ExprEngine` directly from this callback.
- `evalAssume()`. This one's a bummer - it could really benefit from the new functionality, but we can't squeeze a checker context into it because it definitely doesn't make sense to add transitions in the middle of `assume()`. We should probably be able to allow returning a note tag from that callback somehow.
- `checkLiveSymbols()`. I'm not worried about this one because it doesn't correspond to any actual event in the program and there's no way to change the program state at this point. If we want to extract some information from it anyway, i guess we can add opaque checker-specific data into `SymbolReaper` and transfer it to `checkDeadSymbols()`.
- `checkPointerEscape()`, `checkRegionChanges()`. These are usually used for invalidation that normally doesn't deserve a note. But it can be argued that it may deserve a note sometimes (eg., "note: function call might have changed the value of a global variable"), so i guess i'm a tiny bit worried, but we can have a closer look at this when we find any actual examples.
- `checkEndOfTranslationUnit()`, `checkASTDecl()`,`checkASTCodeBody()`. These don't fire during path-sensitive analysis, so there's nothing to worry about.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58367