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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
