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