Skip to content

Expose ReadOnlyNetworkGraph::get_addresses to C by cloning result #1115

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

Conversation

TheBlueMatt
Copy link
Collaborator

@TheBlueMatt TheBlueMatt commented Oct 12, 2021

We cannot expose ReadOnlyNetworkGraph::get_addresses as is in C as
it returns a list of references to an enum, which the bindings
dont support. Instead, we simply clone the result so that it
doesn't contain references.

There's a few options for how to deal with this, one suggested here. We could:

  • take this as-is,
  • have a cfg-flag for building bindings which switches the method's return type and adds the clone,
  • eat the cost and just always clone.

I admittedly don't actually feel super strongly for any of them, just have one here to demonstrate.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #1115 (d2b7f6c) into main (d665748) will increase coverage by 0.37%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1115      +/-   ##
==========================================
+ Coverage   90.67%   91.05%   +0.37%     
==========================================
  Files          66       66              
  Lines       34684    36648    +1964     
==========================================
+ Hits        31449    33369    +1920     
- Misses       3235     3279      +44     
Impacted Files Coverage Δ
lightning/src/routing/network_graph.rs 92.28% <0.00%> (+1.02%) ⬆️
lightning/src/ln/functional_tests.rs 97.84% <0.00%> (+0.40%) ⬆️
lightning/src/ln/onion_utils.rs 95.33% <0.00%> (+0.41%) ⬆️
lightning/src/ln/onion_route_tests.rs 97.76% <0.00%> (+0.66%) ⬆️
lightning/src/ln/functional_test_utils.rs 97.03% <0.00%> (+1.94%) ⬆️
lightning/src/util/events.rs 34.15% <0.00%> (+2.09%) ⬆️
lightning/src/ln/channelmanager.rs 87.81% <0.00%> (+2.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d665748...d2b7f6c. Read the comment docs.

@jkczyz
Copy link
Contributor

jkczyz commented Oct 12, 2021

I'm somewhat partial to cloning. I don't see this used in the code. How do we expect it to be used by users?

@TheBlueMatt
Copy link
Collaborator Author

I'm somewhat partial to cloning

Yea, fair enough, I don't think its worth being annoying about performance here - users shouldn't really be querying this aggressively...

I don't see this used in the code. How do we expect it to be used by users?

The #[doc(hidden)] is entirely ignored by the C bindings generation, so the function is exposed as normal.

We cannot expose ReadOnlyNetworkGraph::get_addresses as is in C as
it returns a list of references to an enum, which the bindings
dont support. Instead, we simply clone the result so that it
doesn't contain references.
@TheBlueMatt TheBlueMatt force-pushed the 2021-10-expose-addr-vec branch from 61c6436 to d2b7f6c Compare October 13, 2021 01:19
@TheBlueMatt TheBlueMatt changed the title Add a second variant of ReadOnlyNetworkGraph::get_addresses for C Expose ReadOnlyNetworkGraph::get_addresses to C by cloning result Oct 13, 2021
@TheBlueMatt
Copy link
Collaborator Author

Changed to always cloning the result and exposing the original function, I think its cleaner.

@TheBlueMatt TheBlueMatt merged commit 2144166 into lightningdevkit:main Oct 13, 2021
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.

3 participants