Skip to content

Java/C++/C#: Bugfix for field flow through reverse read. #2473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 29, 2019

Conversation

aschackmull
Copy link
Contributor

The restriction of the read relation to only those fields that had a storeStep made sense before we added support for treating reverse reads as stores, but it should have been removed when we added that step.

Without this change the added test case would only exhibit flow to the first of the four sinks.

The first pruning step of the data-flow implementation already restricts reads to those for which a corresponding store has been seen during forward flow, so if no reverse reads are encountered this change shouldn't add any tuples to the data-flow pruning sequence.

@aschackmull aschackmull added this to the 1.23 milestone Nov 29, 2019
@aschackmull aschackmull requested review from a team as code owners November 29, 2019 08:47
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

@p0 Are you happy with putting this in 1.23?

@p0
Copy link
Contributor

p0 commented Nov 29, 2019 via email

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@p0
Copy link
Contributor

p0 commented Nov 29, 2019

It'd be nice to add my test case for C++ too...

@jbj
Copy link
Contributor

jbj commented Nov 29, 2019

It'd be nice to add my test case for C++ too...

I can do that in a separate PR so we don't have to wait for the C# tests again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants