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

New modifications #155

Closed
wants to merge 24 commits into from
Closed

New modifications #155

wants to merge 24 commits into from

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Jun 15, 2019

Fixes #154 and some other things.

  • Adds the alloc feature and doesn't remove the std feature because of std::io::Write.
  • Replaces mem::uninitialized with MaybeUninit.
  • Uses NonNull<T> instead of *mut T.
  • Removes deprecated VecLike.
  • Uses core::hint::unreachable_unchecked instead of unchecked.
  • Splits bloated lib.rs file into smaller modules for better readability, localization and maintainability.
  • Bumps minimum compiler version to 1.36.
  • Uses the new 2018 epoch version.
  • Adds new scripts for testing in development and CI.

This change is Reviewable

@c410-f3r c410-f3r force-pushed the master branch 2 times, most recently from c2acd16 to 0865b42 Compare June 15, 2019 18:21
@jdm
Copy link
Member

jdm commented Jun 15, 2019

Could you please redo these changes so there's a separate commit for each of the points you mentioned? This is really difficult to review in its current condition.

@c410-f3r c410-f3r force-pushed the master branch 2 times, most recently from aa1a49c to a797f80 Compare June 15, 2019 18:26
@c410-f3r
Copy link
Contributor Author

@jdm Sure

lib.rs: Alloc feature
Cargo.toml: New features and 2018 edition
lib.rs Outdated
@@ -258,25 +258,25 @@ impl<A: Array> SmallVecData<A> {
#[inline]
unsafe fn inline(&self) -> &A {
match *self {
SmallVecData::Inline(ref a) => a,
SmallVecData::Inline(ref a) => &*a.as_ptr(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it's unsound to return an &A that points to a possibly-uninitialized A. This method should be changed to return a raw pointer. (The callers of this method only need raw pointers anyways.)

lib.rs Outdated
_ => debug_unreachable!(),
}
}
#[inline]
unsafe fn inline_mut(&mut self) -> &mut A {
match *self {
SmallVecData::Inline(ref mut a) => a,
SmallVecData::Inline(ref mut a) => &mut *a.as_mut_ptr(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like inline above, this method should be changed to return a raw pointer.

@c410-f3r
Copy link
Contributor Author

Thanks @mbrubeck

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 4, 2019

1.36 was released and Travis now checks all variants of Rust.

Can I squash commits?

@jdm
Copy link
Member

jdm commented Jul 4, 2019

@SimonSapin How does this work relate to #157?

@SimonSapin
Copy link
Member

I haven’t looked at the diff, but based on the description a lot of it is similar or equivalent. Only with a different approach: incremental changes v.s. rewrite.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 4, 2019

Based on this current work, I can apply @SimonSapin modifications in future PRs

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 8, 2019

The const_generics feature is now fully functional. Is anything preventing this PR from being merged?

@SimonSapin
Copy link
Member

SimonSapin commented Jul 8, 2019

While experimenting with const generics in a branch is fine, I would prefer not to have them in the master branch or on a crates.io release of this crate, at this time.

Constant generics are now fully working.

I realize this is not exactly what you meant, but that’s an overstatement.

Using #![feature(const_generics)] makes current nigthlies print warning: the feature `const_generics` is incomplete and may cause the compiler to crash. As far as I know this kind of warning is unprecedented in Rust. smallvec’s tests might be passing with const generic now, but there is no telling whether it’ll regress in a future Nightly.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 8, 2019

I realize this is not exactly what you meant, but that’s an overstatement.

I am sorry for the misunderstanding. Early versions of this PR contained a non-functional implementation for constant generics and it is now properly fixed.

As far as I know this kind of warning is unprecedented in Rust. smallvec’s tests might be passing with const generic now, but there is no telling whether it’ll regress in a future Nightly.

It is the own nature of unstable Rustc features to break existing code in future releases, the same reason can be applied to already included unstable features of smallvec. If CI testing is a concern, a dedicated job that is allowed to fail can be included.
The current code is structured in a way that no constant generic stuff is invoked without explicitly telling otherwise by setting the feature, therefore, everything on stable will work without problems.

Nevertheless, if you guys feel comfortable enough to merge this code without constant generics, I can extract it into a separate PR.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 31, 2019

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #159) made this pull request unmergeable. Please resolve the merge conflicts.

@emilio
Copy link
Member

emilio commented Oct 19, 2019

I think most of this stuff has landed already incrementally... Should this be closed?

@emilio
Copy link
Member

emilio commented Oct 19, 2019

(Like, constant generics and other improvements are still welcome I guess, but this PR as is is pretty unlikely to land)

@mbrubeck
Copy link
Collaborator

I cherry-picked and landed some of the changes from this that weren't covered by other PRs. I'm closing this now but feel free to open new PRs for any remaining changes. Smaller, incremental patches are appreciated if possible.

Thanks!

@mbrubeck mbrubeck closed this Oct 28, 2019
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.

Constant generic
6 participants