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 Capacity/length methods for OsString. #31608

Merged
merged 1 commit into from
Feb 20, 2016

Conversation

frewsxcv
Copy link
Member

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nikomatsakis
Copy link
Contributor

Looks pretty good to me, but it'd be good to add some tests I think (particular of clear, which has an obvious visible effect, but ideally you'd test the other methods too using unsafe code).

@alexcrichton
Copy link
Member

Looks good to me as well! r=me with some tests like @nikomatsakis mentioned

@frewsxcv
Copy link
Member Author

but ideally you'd test the other methods too using unsafe code

I know what unsafe means in the context of Rust, but I'm not sure why testing these methods would necessitate the usage of unsafe code? Do you have an example to get a better understanding of what you mean?

@alexcrichton
Copy link
Member

I believe @nikomatsakis means testing the methods which themselves internally use unsafe (generally a good idea)

@frewsxcv frewsxcv force-pushed the osstring-simple-functions branch 2 times, most recently from 160b9de to 90c5077 Compare February 19, 2016 14:16
@frewsxcv
Copy link
Member Author

Let me know how those tests look

@frewsxcv frewsxcv force-pushed the osstring-simple-functions branch from 90c5077 to 85aaa7b Compare February 19, 2016 14:17
@alexcrichton
Copy link
Member

@bors: r+ 85aaa7b6df386d26509e6d4bebe9dd3246352510

Thanks!

@bors
Copy link
Contributor

bors commented Feb 19, 2016

⌛ Testing commit 85aaa7b with merge 90c2f30...

@bors
Copy link
Contributor

bors commented Feb 19, 2016

💔 Test failed - auto-win-msvc-32-opt

@frewsxcv frewsxcv force-pushed the osstring-simple-functions branch from 85aaa7b to 8c3655b Compare February 19, 2016 19:05
@frewsxcv
Copy link
Member Author

Forgot to add a few intermediate methods (via Buf) for Windows. Just forced pushed the changes.

@alexcrichton
Copy link
Member

@bors: r+ 8c3655b

@bors
Copy link
Contributor

bors commented Feb 19, 2016

⌛ Testing commit 8c3655b with merge c2aa084...

@alexcrichton
Copy link
Member

@bors: retry force

@bors
Copy link
Contributor

bors commented Feb 19, 2016

⌛ Testing commit 8c3655b with merge 0cb2a7a...

@bors
Copy link
Contributor

bors commented Feb 19, 2016

💔 Test failed - auto-linux-32-opt

@frewsxcv
Copy link
Member Author

Test fail looks unrelated

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Feb 19, 2016 at 3:16 PM, Corey Farwell notifications@github.com
wrote:

Test fail looks unrelated


Reply to this email directly or view it on GitHub
#31608 (comment).

@bors
Copy link
Contributor

bors commented Feb 20, 2016

⌛ Testing commit 8c3655b with merge 4f3d588...

@bors
Copy link
Contributor

bors commented Feb 20, 2016

💔 Test failed - auto-win-gnu-64-nopt-t

@frewsxcv frewsxcv force-pushed the osstring-simple-functions branch from 8c3655b to 29cac06 Compare February 20, 2016 15:45
@frewsxcv
Copy link
Member Author

Forgot to implement Buf::into_inner on the windows side. Latest force push fixes that.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2016

📌 Commit 29cac06 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 20, 2016

⌛ Testing commit 29cac06 with merge b00ad76...

@bors
Copy link
Contributor

bors commented Feb 20, 2016

💔 Test failed - auto-win-msvc-32-opt

@frewsxcv frewsxcv force-pushed the osstring-simple-functions branch from 29cac06 to 2338d74 Compare February 20, 2016 16:38
@frewsxcv
Copy link
Member Author

Forgot the import. Added it in.

@alexcrichton
Copy link
Member

@bors: r+ 2338d74

@bors
Copy link
Contributor

bors commented Feb 20, 2016

⌛ Testing commit 2338d74 with merge 788a21e...

@bors bors merged commit 2338d74 into rust-lang:master Feb 20, 2016
@frewsxcv frewsxcv deleted the osstring-simple-functions branch February 20, 2016 20:51
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.

5 participants