refactor: modularize lib.rs - #458
Conversation
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
Why all the formatting changes at the same time?
| let array = [99; 128]; | ||
| let small_vec: SmallVec<u8, 128> = SmallVec::from(array); | ||
| assert_eq!(&*small_vec, vec![99u8; 128].as_slice()); | ||
| assert_eq!(&*small_vec, Vec::from([99u8; 128]).as_slice()); |
There was a problem hiding this comment.
It's weird that changes like this are included.
There was a problem hiding this comment.
that's because rustfmt misunderstands the imports and it thinks that alloc::vec (importing the macro) would be the same as doing alloc::vec::{self} which is not, that's the module
it only happens when it's told to group imports
| bytes = { version = "1", optional = true, default-features = false } | ||
| serde_core = { version = "1.0.221", optional = true, default-features = false } | ||
| malloc_size_of = { version = "0.1.1", optional = true, default-features = false } | ||
| add-syntax = "0.1.0" |
There was a problem hiding this comment.
that's to make this simpler
#[cfg_attr(feature = "internals", prepend(pub))]
use {
rawsmallvec::RawSmallVec,
taggedlen::TaggedLen
};instead of having two exactly-equal blocks but one with pub and feature flag and the other with negative feature flag and not pub
it's DRY
There was a problem hiding this comment.
I don't think it's worth another dependency. I propose removing that cfg and adding a separate block like this for clarity:
#[cfg(feature = "internals")]
pub use { self::RawSmallVec, self::TaggedLen };
| style_edition = "2024" | ||
| trailing_comma = "Never" | ||
| use_try_shorthand = true | ||
| where_single_line = true No newline at end of file |
There was a problem hiding this comment.
I would prefer to review these changes separately.
There was a problem hiding this comment.
fair, reverted changes on rustfmt.toml
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
I have no issue with the restructuring, but I would like it to happen by itself.
| // Standard Rust vectors are already specialized. | ||
| SmallVec::<T, N>::from_vec(vec![elem; n]) | ||
| // Standard Rust iterators are already specialized. | ||
| repeat_n(elem, n).collect() |
There was a problem hiding this comment.
Can we revert all changes that are not moving code around? It makes reviewing this PR require much more time.
There was a problem hiding this comment.
sorry, that was because I had to remove the macro
it's just a few of those changes, not much, all else was formatting (the other one was making taggedlen public, and the methods of rawsmallvec public as well)
I don't think I can revert them all without engineering the diff
I'd be better throwing the branch and creating a new one
There was a problem hiding this comment.
I know it's annoying, but I really want to review focused PRs. A change that describes a new repo structure and just moves code between files is easy to review. A change that does several things at once is not.
There was a problem hiding this comment.
will open a new PR
2 participants
closes #478
scope of this PR
the PR offloads the following files from
lib.rs:bytes.rs=> the bytes feature and its implementationscomparisons.rs=>EqandOrd, total and partialmallocsizeof.rs=> regarding the feature malloc_size_ofrawsmallvec.rs=> functionality regarding the raw typereferences.rs=> deref, asref, borrowserde.rs=> serialize / deserializetaggedlen.rs=> the tagged lengthexports
TaggedLenwith the internals feature and makesRawSmallVecmethods publicmodifies
rustfmt.tomla bit to make the style more consistent across the codebasesaves ~20 LOC