Skip to content

Java/C++/C#: Provide path-node locations via hasLocationInfo, not getLocation.#1729

Merged
hvitved merged 2 commits into
github:masterfrom
xiemaisi:data-flow-nodes-location
Aug 13, 2019
Merged

Java/C++/C#: Provide path-node locations via hasLocationInfo, not getLocation.#1729
hvitved merged 2 commits into
github:masterfrom
xiemaisi:data-flow-nodes-location

Conversation

@xiemaisi

Copy link
Copy Markdown

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

@hvitved hvitved left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

hvitved commented Aug 12, 2019

Copy link
Copy Markdown
Contributor

@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.

@jbj jbj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@xiemaisi

Copy link
Copy Markdown
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