Skip to content

Conversation

@Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Jan 9, 2021

PacketBuffer currently requires 2 different lifetimes for metadata and payload buffers. This causes a lot of cascading complexity causing Sockets to require 2 lifetimes and SocketSet to require 3 (!) lifetimes.

I can't think of any usage requiring 2 separate lifetimes in PacketBuffer. (ie something you could do with separate lifetimes but can't do with just 1).

This PR removes it, yielding a nice simplification of lifetimes everywhere :)

@Dirbaio Dirbaio force-pushed the simplify-lifetimes branch from e8e9500 to 11255a8 Compare January 9, 2021 00:59
@Dirbaio Dirbaio merged commit 0d120af into master Jan 9, 2021
@Dirbaio Dirbaio deleted the simplify-lifetimes branch January 9, 2021 01:04
@whitequark
Copy link
Contributor

Are you completely sure? Our examples don't test no_std lifetimes nearly enough.

@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 9, 2021

Yes!

With the new code, when creating a PacketBuffer with buffers that could have different lifetimes (for example, one 'static and one 'foo), its 'a lifetime becomes the smaller lifetime, which is 'foo. The end result is the PacketBuffer can only live for 'foo.

With the previous code, the PacketBuffer would get 'a = 'static and 'b = 'foo. The struct can only live for the min of all its lifetime params. The end result is the PacketBuffer can only live for 'foo, again.

The only way single-vs-multiple lifetimes can make a difference is if there are APIs to "take out" the &muts, or to "move" them somewhere else that's not the struct. If there's a single lifetime, all the &muts are "downgraded" to the smallest lifetime because the information is "lost", if there are multiple each individual lifetime is kept. We don't have such APIs in PacketBuffer or Socket though.

@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 9, 2021

I also think similar simplifications can be made for Interface and SocketSet too.

@whitequark
Copy link
Contributor

I thought it was necessary, but it seems like I misunderstood exactly how lifetimes work.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants