Conversation
Robbepop
left a comment
Robbepop
left a comment
There was a problem hiding this comment.
I personally dislike the bitvec crate for being overcomplicated but if it is used pervasively by Polkadot/Sustrate and is thereby supported by parity_scale_codec we should also support it. Implementation looks fine to me.
src/ty/mod.rs
Outdated
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| #[cfg_attr(any(feature = "std", feature = "decode"), derive(scale::Decode))] | ||
| #[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Encode, Debug)] | ||
| pub struct TypeDefBitVec<T: Form = MetaForm> { |
There was a problem hiding this comment.
I'd rename this to TypeDefBitSequence to match with Sequence type definition.
src/ty/mod.rs
Outdated
| #[cfg(feature = "bit-vec")] | ||
| /// A BitVec type. | ||
| BitVec(TypeDefBitVec<T>), |
There was a problem hiding this comment.
I think dtolnay mentioned that for compatibility with respect to different crate feature sets it is better to replace the inner type of BitVec with a placeholder type (dummy) of not(feature = "bit-vec") instead of entirely removing this variant.
There was a problem hiding this comment.
That is much better, will avoid issues with incompatible decoding if we add variants after this.
There was a problem hiding this comment.
I've removed the feature flag from this variant now so it will be binary compatible, and also so that decoding/deserializing is supported even without the feature enabled.
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
Maybe you could move the bitvec stuff into it's own module (and file) to cut down on the #[cfg] noise a bit?
Either way, lgtm.
3 participants
Required for
polkadot, some types useBitVece.g. https://github.com/paritytech/polkadot/blob/master/primitives/src/v1/mod.rs#L468.Rationale is that
parity-scale-codechas support for bitvec: https://github.com/paritytech/parity-scale-codec/blob/master/src/bit_vec.rs, by enabling thebit-vecfeature, so this library should too.EDIT
I've modified this so that the
TypeDefBitSequencestruct and variant is still included even withoutbit-vecenabled, in order for compatible encoding/decoding between the crate with or without the feature enabled. Enabling the feature will simply add thebitvecdependency and the relevantTypeInfoimpls.