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.

Sign up for GitHub

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

Jump to bottom

Evaluate migration of SmallVec to min_const_generics #598

Merged
merged 18 commits into from
Feb 4, 2021
Merged

Evaluate migration of SmallVec to min_const_generics #598

merged 18 commits into from
Feb 4, 2021

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Nov 26, 2020 •
edited
Loading

Closes #588.

@cmichi cmichi requested a review from Robbepop November 26, 2020 21:43
@cmichi cmichi changed the title Migrate SmallVec to min_const_generics Evaluate migration of SmallVec to min_const_generics Nov 26, 2020
@cmichi cmichi force-pushed the cmichi-migrate-smallvec-to-const-fn branch from c4dba5f to b58062d Compare November 26, 2020 22:12
Robbepop
Robbepop requested changes Dec 2, 2020
View reviewed changes
crates/storage/src/lazy/cache_cell.rs Outdated Show resolved Hide resolved
crates/storage/src/lib.rs Outdated
@@ -20,6 +20,7 @@
//! FFI to interface with SRML contracts and a primitive blockchain
//! emulator for simple off-chain testing.

#![feature(min_const_generics)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR is mergable as soon as this feature is stabilizied.

crates/storage/src/lazy/lazy_array.rs Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Dec 2, 2020 •
edited
Loading

Codecov Report

Merging #598 (ddb478d) into master (9d47ffa) will decrease coverage by 16.53%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #598       +/-   ##
===========================================
- Coverage   83.76%   67.23%   -16.54%     
===========================================
  Files         156      155        -1     
  Lines        6971     6915       -56     
===========================================
- Hits         5839     4649     -1190     
- Misses       1132     2266     +1134
Impacted Files Coverage Δ
crates/storage/src/collections/smallvec/impls.rs 95.65% <ø> (ø)
crates/storage/src/collections/smallvec/iter.rs 100.00% <ø> (ø)
crates/storage/src/collections/smallvec/mod.rs 96.47% <ø> (ø)
crates/storage/src/collections/smallvec/storage.rs 100.00% <ø> (ø)
crates/storage/src/lazy/cache_cell.rs 100.00% <ø> (ø)
crates/storage/src/lazy/mod.rs 93.54% <ø> (ø)
crates/storage/src/collections/smallvec/tests.rs 100.00% <100.00%> (ø)
crates/storage/src/lazy/lazy_array.rs 96.19% <100.00%> (ø)
crates/env/src/topics.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/traits.rs 0.00% <0.00%> (-100.00%) ⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a50bf6d...f35e08f. Read the comment docs.

@cmichi
Copy link
Collaborator Author

cmichi commented Dec 3, 2020

@Robbepop Today I noticed that there already is a feature const-generics in the code for array-init. It does exactly what we need and I temporarily added it to this PR and migrated the code to use it. I talked to the maintainer and he plans to make it available by default as soon as min_const_generics are stabilized.

Yeah so overall we just need to wait until it's stabilized, remove the feature flags from this PR and are good to merge then.

I've marked the PR as blocked until then.

@cmichi cmichi added the E-blocked The task is blocked on some other task to be finished. label Dec 3, 2020
Robbepop
Robbepop reviewed Dec 3, 2020
View reviewed changes
crates/storage/src/lib.rs Outdated Show resolved Hide resolved
cmichi
cmichi commented Feb 2, 2021
View reviewed changes
crates/storage/Cargo.toml
@@ -25,8 +25,7 @@ scale = { package = "parity-scale-codec", version = "2.0", default-features = fa
derive_more = { version = "0.99", default-features = false, features = ["from", "display"] }
scale-info = { version = "0.5", default-features = false, features = ["derive"], optional = true }
cfg-if = "1.0"
array-init = "1.0"
generic-array = "0.14.1"
array-init = { version = "1.0", default-features = false, features = ["const-generics"] }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We still have to pass this feature, since this dependency will only make it available be default once min_const_generics is in a stable release.

Copy link
Collaborator

@Robbepop Robbepop Feb 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

hmmm good point.
best would be to sync array-init with us or vice versa.
good thing we no longer depend on generic-array with its tons of unsafe code.

@Robbepop
Copy link
Collaborator

Robbepop commented Feb 4, 2021

We concluded to push this PR forward even though min_const_generics has not yet been stabilized.
Since we do not have a guarantee that min_const_generics is actually shipping with Rust version 1.51 in 25th of march we will put the LazyArray as well as everything that depends on it (namely ink_storage::collections::SmallVec) behind the new ink-unstable crate feature for the ink_storage crate.
This way users can decide themselves if they want to depend on this feature (as opt-in) and therefore risk being forced to nightly Rust for a longer time.

The big advantage is that as soon as min_const_generics is stabilized we can simply release the new SmallVec and LazyArray data structures without breaking user code.

Robbepop
Robbepop approved these changes Feb 4, 2021
View reviewed changes
Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@cmichi cmichi merged commit 3d337f4 into master Feb 4, 2021
@cmichi cmichi deleted the cmichi-migrate-smallvec-to-const-fn branch February 4, 2021 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Robbepop Robbepop Robbepop approved these changes

Assignees
No one assigned
Labels
E-blocked The task is blocked on some other task to be finished.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Evaluate usage of min_const_generics for SmallVec
3 participants
@cmichi @codecov-io @Robbepop