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

Deprecated/moved functions in collections::str #15426

Merged
merged 8 commits into from
Jul 15, 2014
Merged

Deprecated/moved functions in collections::str #15426

merged 8 commits into from
Jul 15, 2014

Conversation

aochagavia
Copy link
Contributor

  • Deprecated str::from_utf8_owned in favor of String::from_utf8
  • Deprecated str::from_utf8_lossy in favor of String::from_utf8_lossy
  • Deprecated str::from_utf16 in favor of String::from_utf16
  • Deprecated str::from_utf16_lossy in favor of String::from_utf16_lossy
  • Deprecated str::from_chars in favor of String::from_chars
  • Deprecated str::from_char in favor of String::from_char and .to_string()
  • Deprecated str::from_byte in favor of String::from_byte

[breaking-change]

@aochagavia
Copy link
Contributor Author

And it finaly passes the Travis build!

@alexcrichton
Copy link
Member

This is changing a core standard library modules, so it should be discussed a bit before merging.

I'm in favor of all changes made here.

cc @aturon

@aochagavia
Copy link
Contributor Author

Are there any news on this? Should I rebase it?

@aturon
Copy link
Member

aturon commented Jul 10, 2014

@aochagavia Sorry about the delay. This is my fault -- the CC from Alex got lost in my inbox!

I am definitely in favor of this change. I would also like to see the from_utf16, from_utf16_lossy and from_utf8_lossy functions move to string.

The MaybeOwned type is a split personality between String and &str, so from_utf8_lossy can reasonably live in either module -- but I would argue it should live in string so that all of these related functions are available from a single spot.

@alexcrichton
Copy link
Member

Looks like this needs a rebase, but r=me with a rebase.

@aochagavia
Copy link
Contributor Author

Done 😉!

@aochagavia
Copy link
Contributor Author

This should now be fixed.

@aochagavia
Copy link
Contributor Author

I have also fixed that one... No idea why it hasn't been catched by the Travis build...

@alexcrichton
Copy link
Member

The travis builds only exercise functionality on linux, and the bugs have been on windows/osx so these are likely platform-specific modules.

@aochagavia
Copy link
Contributor Author

I could fix this by using transmute, but it looks very hacky... Is there a better alternative?

@alexcrichton
Copy link
Member

For the collections tests I would recommend ensuring that the right instance of Vec is imported for the tests. Also, could you be sure to run the entire test suite locally before asking for another r+? Refactorings like this often lead to tricky-to-find bugs.

@aochagavia
Copy link
Contributor Author

I have compiled and tested the collections library with rustc lib.rs --test. All tests pass now.

@alexcrichton
Copy link
Member

Just to be sure, have you run a full make check? There are many surprises other than just libcollections!

@aochagavia
Copy link
Contributor Author

Yes I have, most test pass but I got a strange error when running bootstrap-from-c-with-green. I think it isn't related to the changes I made though:

/c/Users/aochagavia/Desktop/rust/i686-pc-mingw32/test/run-make/bootstrap-from-c-with-green/main.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory make[1]: *** [all] Error 127

@alexcrichton
Copy link
Member

Errors like that are normally benign, so let's give this another go

@alexcrichton
Copy link
Member

Thanks again of course!

@aochagavia
Copy link
Contributor Author

No problem! I am the one who benefits most from this 😉! I am really learning a lot here.

@aochagavia
Copy link
Contributor Author

Sorry, I forgot to update some of the code examples of the deprecated functions (make check didn't catch it because it failed before testing the docs). Now I have replaced them by a comment saying that the function is deprecated, and pointing to the methods defined in String.

Use `String::from_utf8` instead

[breaking-change]
Use `String::from_chars` instead

[breaking-change]
Use `String::from_char` or `.to_str` instead

[breaking-change]
Replaced by `String::from_byte`

[breaking-change]
Use `String::from_utf16` instead

[breaking-change]
Use `String::from_utf16_lossy` instead.

[breaking-change]
Use `String::from_utf8_lossy` instead

[breaking-change]
@aochagavia
Copy link
Contributor Author

I am sorry it failed again... There were some encoding issues in the code examples (I accidentaly copied some UTF-8 text and pasted it as ASCII), so the assertions failed. It should be ok now.

I tried to test this before pushing, but it is really a pain to do since I am on Windows. The tests fail before reaching the code examples of collections.

bors added a commit that referenced this pull request Jul 15, 2014
* Deprecated `str::from_utf8_owned` in favor of `String::from_utf8`
* Deprecated `str::from_utf8_lossy` in favor of `String::from_utf8_lossy`
* Deprecated `str::from_utf16` in favor of `String::from_utf16`
* Deprecated `str::from_utf16_lossy` in favor of `String::from_utf16_lossy`
* Deprecated `str::from_chars` in favor of `String::from_chars`
* Deprecated `str::from_char` in favor of `String::from_char` and `.to_string()`
* Deprecated `str::from_byte` in favor of `String::from_byte`

[breaking-change]
@bors bors closed this Jul 15, 2014
@bors bors merged commit 584fbde into rust-lang:master Jul 15, 2014
@alexcrichton
Copy link
Member

Awesome \o/

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2023
…_return, r=Veykril

minor : Deunwrap convert_to_guarded_return

Closes subtask 12 of rust-lang#15398
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.

4 participants