NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2871
+ // Overwrite the associated constraint of the Symbol.
+ Constraints = CF.remove(Constraints, Sym);
Constraints = CF.add(Constraints, Sym, C.second);
----------------
xazax.hun wrote:
> martong wrote:
> > martong wrote:
> > > martong wrote:
> > > > steakhal wrote:
> > > > > How can we simplify this?
> > > > Could we change the visitation logic to start with the `EndPathNode`? I
> > > > mean could we exercise `FalsePositiveRefutationBRVisitor` to start the
> > > > visitation from `EndPathNode`? If that was possible then this whole
> > > > patch would be way simpler.
> > > > In BugReporter.cpp:
> > > > ```
> > > > // Run visitors on all nodes starting from the node *before* the last
> > > > one.
> > > > // The last node is reserved for notes generated with {@code
> > > > getEndPath}.
> > > > const ExplodedNode *NextNode = ErrorNode->getFirstPred();
> > > > while (NextNode) {
> > > > ```
> > > > Why can't we have a different visitation order implemented specifically
> > > > for the refutation visitor? (@NoQ, @xazax.hun)
> > > Hmm, to answer my own question, then we would visit the ErrorNode twice.
> > > So then perhaps we should not allow any constraints in the ErrorNodes,
> > > right? What if we'd split all such ErrorNodes into a PrevNode with the
> > > constraints and a simple ErrorNode without the constraints?
> > @xazax.hun, and others: Any thoughts on my comments above? What do you
> > think, would it be possible to split the error node into two nodes? A
> > PrevNode with the constraints and a simple ErrorNode without the
> > constraints?
> I think this would make a lot of sense in the context of this visitor. But we
> would need to check if it also makes sense in the context of the other parts
> of the analyzer (other visitors and mechanisms like bug report generation).
> I'd prefer such a change to be a separate patch so we can assess it easier
> whether it makes things easier or more complicated. I think it might be worth
> to experiment with.
We could add a new visitor callback, like `VisitErrorNode` or something, and
call it before visiting all other nodes. It won't affect other visitors because
they don't subscribe to it. Then drop the update in `finalizeVisitor` in this
visitor. Visitor callbacks are currently messy; it's hard to understand what
exactly they're doing by looking at their name. A cleanup is super welcome.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78457/new/
https://reviews.llvm.org/D78457
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
