Skip to content

Conversation

xiemaisi
Copy link

As discussed, this is slightly more flexible and would make my life a little easier in reusing the shared library (though I could work around it in other ways if this change is considered too disruptive). It also eliminates the only uses of DataFlowLocation, but I'm fine with keeping it around for potential future uses.

cc @hvitved, @jbj, @dave-bartolomeo

@xiemaisi xiemaisi requested a review from aschackmull August 12, 2019 12:01
@xiemaisi xiemaisi requested review from a team as code owners August 12, 2019 12:01
hvitved
hvitved previously approved these changes Aug 12, 2019
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.

aschackmull
aschackmull previously approved these changes Aug 12, 2019
@xiemaisi xiemaisi dismissed stale reviews from aschackmull and hvitved via 485d426 August 12, 2019 14:04
@xiemaisi
Copy link
Author

@hvitved: It turns out the C# library uses PathNode.getLocation() in at least one place. While that's easy to fix (see latest commit) it makes me wonder whether it's perhaps a more widely used predicate than I had thought...

@hvitved
Copy link
Contributor

hvitved commented Aug 12, 2019

@hvitved: It turns out the C# library uses PathNode.getLocation() in at least one place. While that's easy to fix (see latest commit) it makes me wonder whether it's perhaps a more widely used predicate than I had thought...

It appears that this is the only place where it was used, and I don't mind the change from your last commit.

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.

LGTM

@xiemaisi
Copy link
Author

@hvitved / @aschackmull, could you re-approve (or merge straight away)?

@hvitved hvitved merged commit 36043d0 into github:master Aug 13, 2019
@xiemaisi xiemaisi deleted the data-flow-nodes-location branch September 12, 2019 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants