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

Add from_vec method for efficient creation from a Vec. #48

Merged
merged 3 commits into from
Mar 29, 2017
Merged

Add from_vec method for efficient creation from a Vec. #48

merged 3 commits into from
Mar 29, 2017

Conversation

SergioBenitez
Copy link
Contributor

@SergioBenitez SergioBenitez commented Mar 29, 2017

I also bumped the version to 0.3.3 for easy publishing.


This change is Reviewable

@emilio
Copy link
Member

emilio commented Mar 29, 2017

Could you add a few tests for this? In particular, an empty vector pointer may not be an actual valid pointer IIRC (may be 0x1?).

@SergioBenitez
Copy link
Contributor Author

Sure, added.

@emilio
Copy link
Member

emilio commented Mar 29, 2017

Oh, ok, so we don't rely on the validness of the pointer (we just rely on Vec for most of stuff). r=me then

@bors-servo r+

Thanks for doing this!

@bors-servo
Copy link
Contributor

📌 Commit 0bf0141 has been approved by emilio

@bors-servo
Copy link
Contributor

⌛ Testing commit 0bf0141 with merge 46ef708...

bors-servo pushed a commit that referenced this pull request Mar 29, 2017
Add from_vec method for efficient creation from a Vec.

I also bumped the version to 0.3.3 for easy publishing.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/48)
<!-- Reviewable:end -->
@SergioBenitez
Copy link
Contributor Author

Well, we certainly take the raw pointer from Vec, but we do so via the public API. The documentation doesn't say anything about the pointer possibly being invalid.

@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: emilio
Pushing 46ef708 to master...

@bors-servo bors-servo merged commit 0bf0141 into servo:master Mar 29, 2017
@emilio
Copy link
Member

emilio commented Mar 29, 2017

Well, we certainly take the raw pointer from Vec, but we do so via the public API. The documentation doesn't say anything about the pointer possibly being invalid.

Right, there's no guarantee about that AFAIK. See https://bugzilla.mozilla.org/show_bug.cgi?id=1344209, which is the reason I was wary of this.

@SergioBenitez
Copy link
Contributor Author

Looking at the source code for Vec, ptr will indeed be some constant, invalid value when the Vec is empty (alloc::heap::EMPTY). However, the ptr will only be this value when capacity/length are zero or T is a zero-sized type. As a result, SmallVec should never dereference the invalid pointer, and everything should work exactly as expected.

In the bug you linked, it seems that the fix was to use a null pointer when the string is empty. Presumably this is because some of the Core Foundation/Foundation code checks the ptr value for nullness to determine if it can be dereferenced. None of the code in SmallVec does this, another indication that everything is alright.

@emilio
Copy link
Member

emilio commented Mar 29, 2017

Right, it's not about core foundation, but Gecko's code, but that's right :)

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