Skip to content

Conversation

@thomaseizinger
Copy link
Owner

This removes the Unpin requirement introduced in #10 and #11 in favor of returning an Iterator of pinned references instead.

@thomaseizinger
Copy link
Owner Author

@elenaf9 @teor2345 What do you think of this? Is my unsafe code sound?

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I believe it is sound, but the safety requirements are stricter than the current comments. (In all 4 places.)

@teor2345
Copy link
Contributor

Also, this will require a revert/change to the release notes I added in #11

@teor2345
Copy link
Contributor

@thomaseizinger just checking if we can merge this at some point?

@thomaseizinger
Copy link
Owner Author

@thomaseizinger just checking if we can merge this at some point?

Yeah! Has this code been tested to work as expected for rust-libp2p?

@teor2345
Copy link
Contributor

@thomaseizinger just checking if we can merge this at some point?

Yeah! Has this code been tested to work as expected for rust-libp2p?

I haven't had a chance, and I don't know if @elenaf9 has either.

@thomaseizinger
Copy link
Owner Author

Thanks for the review @teor2345, that removed two unsafes! 🎉

I'll go ahead and merge this, the tests are passing here and we have removed the Unpin requirement so I think this is good to go.

@thomaseizinger thomaseizinger enabled auto-merge (squash) July 24, 2025 23:01
@thomaseizinger thomaseizinger merged commit 012803d into main Jul 24, 2025
1 check passed
@thomaseizinger thomaseizinger deleted the fix/no-unpin branch July 24, 2025 23:01
@thomaseizinger
Copy link
Owner Author

Would appreciate some testing in rust-libp2p before we cut the release.

@teor2345
Copy link
Contributor

Would appreciate some testing in rust-libp2p before we cut the release.

Totally understandable, I'll see if I can get work time to do it.

@elenaf9
Copy link
Contributor

elenaf9 commented Jul 25, 2025

Sorry, I didn't have the chance to test this yet either. Will try to find some time this weekend, but if not I'd appreciate your help @teor2345.

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