[PATCH] D103605: [analyzer] Introduce a new interface for tracking

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103605#2796111 <https://reviews.llvm.org/D103605#2796111>, @martong wrote:

> Great initiative! You haven't mention `markInteresting` functions, but I think it would be really important to incorporate that functionality here as well. IMHO, `markInteresting` shouldn't have been part of the public API ever, Checkers should have been calling only the trackExpressionValue (as internally that calls markInteresting anyway).

Oh, yes, interesting-ness doesn't provide a very clear interface for the feedback loop that I'm trying to adjust.  From the perspective of the use "mark interesting whatever your checker is interested in and the core will figure it out", it's fine.  It has a clear message and you simply expect the engine to do its magic.  The problem comes when you try to catch something interesting from your own checker-specific visitor.  In that use case, you, first of all, really need to understand how trackers traverse the graph and what they might consider interesting, and rely on the fact that you will get the right interesting thingy.  So, the predicate checking for interesting-ness available to the users, is not very helpful IMO.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:113
+
+struct StoreInfo {
+  enum Kind {
----------------
martong wrote:
> I think we should document this class' responsibility with a terse sentence.
Sure


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:231
+  template <class ExpressionHandlerType, class... Args>
+  void addExpressionHandlerToTheTop(Args &&... ConstructorArgs) {
+    addExpressionHandlerToTheTop(std::make_unique<ExpressionHandlerType>(
----------------
martong wrote:
> This naming is a bit too bloated to me, what do you think about this:
> ```
> enum class Position { Front, Back };
> addExpressionHandler(Front, ...) {
> ```
Maybe `addHighPriorityHandler` and `addLowPriorityHandler`? There is probably no reason to put `Expression` and `Store` into the actual name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103605