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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D103605#2796141 <https://reviews.llvm.org/D103605#2796141>, @vsavchenko wrote:

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

What I mean is that probably we should have the following function(s) provided for the user:

  Result track(SVal InterestingSVal, ....);
  Result track(const MemRegion* InterestingRegion, ....);
  Result track(SymbolRef InterestingSym, ....);

Or even better,  only `track(SVal InterestingSVal, ....)` would be enough to be public. Then we could deprecate `markInteresting` and all Checkers should use your new Tracker interface. As I see, both trackExpressionValue and markInteresting serves the same purpose: i.e. to do a backward slicing in the ExplodedGraph to select interesing/important nodes that should be presented to the user.



================
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>(
----------------
vsavchenko wrote:
> vsavchenko wrote:
> > 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.
> What do you think about this solution?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103605