Skip to content
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

rlp: add ArrayVec impls #432

Closed
wants to merge 1 commit into from
Closed

rlp: add ArrayVec impls #432

wants to merge 1 commit into from

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Oct 17, 2020

Useful for limited-size lists.

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

A few tests would be good: normal, zero-length, beyond capacity.

@@ -79,6 +79,44 @@ impl Decodable for Vec<u8> {
}
}

#[cfg(feature = "arrayvec")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you missed adding this feature to Cargo.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optional dependencies are features in themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL! 👍

(I'd still want all features in the Cargo.toml, preferably documented, for discoverability reasons)

@dvdplm
Copy link
Contributor

dvdplm commented Oct 17, 2020

I don't want to be naysayer but this is a slippery slope here; ArrayVec yes but SmallVec, TinyVec, ThinVec no? It would be good to have a concrete use-case and a proper motivation for why ArrayVec will be useful long-term.

@vorot93
Copy link
Contributor Author

vorot93 commented Oct 17, 2020

My concrete use case is for Neighbours discv4 packet:

https://github.com/ethereum/devp2p/blob/master/discv4.md#neighbors-packet-0x04

It is a given that the reply is a limited-size vec of up to 16 elements. Would be nice to embed this property in the type system, but for that I need RLP to support any limited-size vec implementation. I suppose ArrayVec is the most popular out there, so I just went with that.

@dvdplm
Copy link
Contributor

dvdplm commented Oct 18, 2020

Thank you for the motivation, it's much clearer now. I'm still hesitant. Even if it's an optional dependency, it still needs maintenance. The future of arrayvec is a bit unclear, as the author puts it "Const generics is arrayvec 2.0", so I think this would be useful mostly until const generics is ready?

@ordian
Copy link
Member

ordian commented Nov 30, 2020

@vorot93 is this PR still relevant now that we have Bytes impl?

@vorot93
Copy link
Contributor Author

vorot93 commented Nov 30, 2020

@ordian Yes! This PR is about limited collections of items, not byte arrays.

At the same time, however, I can see @dvdplm 's reasoning that eventually ArrayVec may be made obsolete by const generics.

@ordian
Copy link
Member

ordian commented Nov 30, 2020

I see, I thought the reason was to avoid allocations which, it seems, you were able to achieve with bytes.
I share the same opinion as @dvdplm and hesitant to merge this as min const generics are targeting stabilization in february.
For now you can use a newtype wrapper for ArrayVec.

@vorot93 vorot93 closed this Nov 30, 2020
@vorot93 vorot93 deleted the arrayvec branch November 30, 2020 19:31
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.

None yet

3 participants