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

"try_*" functions and "to_vec" for arrayvec #82

Merged
merged 17 commits into from
Jul 17, 2020
Merged

Conversation

Soveu
Copy link
Contributor

@Soveu Soveu commented Jul 15, 2020

Plus, a bit of rewrite of tinyvec to use them.

@Soveu
Copy link
Contributor Author

Soveu commented Jul 15, 2020

I'm thinking if to_vec is a good name, because it consumes the items.
Maybe into_vec could be a better choice.

@Lokathor
Copy link
Owner

Well for one thing it looks like you added no tests at all >_>

@ThatsNoMoon
Copy link
Contributor

@Soveu I have a reserve (and reserve_exact) test I made before I realized you were making this PR; you could take that from my fork.

@Soveu
Copy link
Contributor Author

Soveu commented Jul 17, 2020

From Vec::reserve_exact

Note that the allocator may give the collection more space than it requests. 
Therefore, capacity can not be relied upon to be precisely minimal. 
Prefer reserve if future insertions are expected.

So technically tests for reserve_exact should be the same as for exact, capacity() >= amount_requested
Anyway, thanks for them, @ThatsNoMoon

@Soveu
Copy link
Contributor Author

Soveu commented Jul 17, 2020

Appveyor test died, because of matches! macro :/

@Lokathor
Copy link
Owner

Lokathor commented Jul 17, 2020

ah, yeah, gotta be really strict on keeping to a minimum version im afraid.

@Soveu
Copy link
Contributor Author

Soveu commented Jul 17, 2020

You can merge this PR (with #80) if you don't have any objections.
I'm okay with the license, it doesn't change much for me anyway 😉

@Lokathor
Copy link
Owner

okay, 80 first, then this, then we can update the 3rd PR to use reserve.

@Lokathor
Copy link
Owner

oh i forgot that i haven't reviewed this yet. I'll have to sit down and look closely later today.

In the mean time, it looks like merging #80 made some conflicts here.

@Soveu
Copy link
Contributor Author

Soveu commented Jul 17, 2020

one conflict was because I left previously just one new line in tests/tinyvec.rs :D

@Soveu
Copy link
Contributor Author

Soveu commented Jul 17, 2020

I am literally now sitting in github file editor, hoping that it will work 🙃

@Soveu
Copy link
Contributor Author

Soveu commented Jul 17, 2020

Now I understand the merge conflict joke

@Soveu Soveu mentioned this pull request Jul 17, 2020
@Soveu
Copy link
Contributor Author

Soveu commented Jul 17, 2020

An error occurred while generating the build script. from Travis CI :(

@Lokathor
Copy link
Owner

there's already a take function in lib.rs actually

@Lokathor
Copy link
Owner

CI seems happy now. Travis is just flaky sometimes.

@Lokathor Lokathor merged commit 3919d5e into Lokathor:main Jul 17, 2020
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.

3 participants