Page MenuHomePhabricator

[analyzer] Refactor trackRValueExpression into ExpressionHandler
ClosedPublic

Authored by vsavchenko on Jun 3 2021, 9:33 AM.

Diff Detail

Event Timeline

vsavchenko created this revision.Jun 3 2021, 9:33 AM
vsavchenko requested review of this revision.Jun 3 2021, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2021, 9:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ accepted this revision.Jun 9 2021, 9:57 PM

I guess you should mark all of these commits as NFC?

This revision is now accepted and ready to land.Jun 9 2021, 9:57 PM

I guess you should mark all of these commits as NFC?

I thought about, but they do change one thing: return value of trackExpressionValue. With each commit it's getting more consistent.

This revision was landed with ongoing or failed builds.Jun 11 2021, 2:52 AM
This revision was automatically updated to reflect the committed changes.
Szelethus added inline comments.Jun 14 2021, 5:53 AM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2263–2264

I checked this commit out, and failed to see how these low/high priority handlers work out in practice. I tried to

  • Make the default handler low prio as well
  • Make the PRValue high prio
  • All other combinations of the those values
  • Change the order of adding these handlers

No tests failed. Is there any one patch so far that demonstrates why we need this? A unit test?

vsavchenko added inline comments.Jun 14 2021, 6:02 AM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2263–2264

add with high priority == add to the beginning of the queue
add with low priority == add to the end of the queue

This means that it doesn't matter with what priority we add the very first one.
I guess with these two it doesn't matter which order they have one against another, that's why tests don't fail. This code here keeps the same order it was originally in trackExpressionValue to maintain parity.

In the future commits, InterestingLValueHandler HAVE TO BE before before DefaultExpressionHandler and there are tests that validate it. Actually this is the only place that I know that really cares about the order and that's why I needed to add priorities and the way for one handler to cancel all further handlers.

Does this answer your question?

Szelethus added inline comments.Jun 14 2021, 6:14 AM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2263–2264

Perfectly, thank you! :)