C++: Support nested field flow#1772
Conversation
jbj
force-pushed
the
ast-field-flow-nested
branch
from
84e6243 to
da49e24
Compare
|
The FPs introduced in this PR are an artifact of the test being a bit artificial. They are fixed by #1797, which is under consideration in Copenhagen. |
| exists(PartialDefinition pd | | ||
| node = pd.getSubBasicBlockStart() and | ||
| fa = pd.getDefinedExpr() | ||
| ) |
There was a problem hiding this comment.
I'm struggling with this as the comments explaining PartialDefinitions are rather minimal. Perhaps we could talk through an example?
There was a problem hiding this comment.
Thanks, I understand what's going on now.
| // path somewhere in the search. That makes the library conclude that there | ||
| // could be flow to `b.f.a_` even when the flow was actually to `b.f.b_`. | ||
| sink(b.f.a()); // flow [FALSE POSITIVE through `b2.f.setB` and `b3.f.setB`] | ||
| sink(b.f.b()); // flow [FALSE POSITIVE through `b1.f.setA` and `b3.f.setA`] |
There was a problem hiding this comment.
It would be nice to have a test case where the changes do get the correct result.
There was a problem hiding this comment.
When we discussed this, @jbj pointed out that there are correct results on these lines as well as false positive results (and he expects future changes to eliminate the FPs here).
| exists(PartialDefinition pd | | ||
| node = pd.getSubBasicBlockStart() and | ||
| fa = pd.getDefinedExpr() | ||
| ) |
There was a problem hiding this comment.
Thanks, I understand what's going on now.
| // path somewhere in the search. That makes the library conclude that there | ||
| // could be flow to `b.f.a_` even when the flow was actually to `b.f.b_`. | ||
| sink(b.f.a()); // flow [FALSE POSITIVE through `b2.f.setB` and `b3.f.setB`] | ||
| sink(b.f.b()); // flow [FALSE POSITIVE through `b1.f.setA` and `b3.f.setA`] |
There was a problem hiding this comment.
When we discussed this, @jbj pointed out that there are correct results on these lines as well as false positive results (and he expects future changes to eliminate the FPs here).
|
There's now a conflict, other than this I'm ready to merge this PR. |
This is the C/C++ side of PR github#1766.
jbj
force-pushed
the
ast-field-flow-nested
branch
from
da49e24 to
d3a6ae5
Compare
2 participants
This is the C/C++ side of #1766.