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

Move into_ascii_{low,upp}ercase (back) to a separate trait. #32076

Closed
wants to merge 1 commit into from

Conversation

SimonSapin
Copy link
Contributor

Implementing AsciiExt for String and Vec<u8> caused a regression: #32074 and the where Self: Sized hack to have the into_ methods in that trait (which is also implemented for DST str and [u8]) was rather clunky.

CC #27809

r? @alexcrichton

@Manishearth Manishearth added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 6, 2016
@SimonSapin
Copy link
Contributor Author

This should be backported to 1.8 beta to fix the #32074 regression there.

@bors
Copy link
Contributor

bors commented Mar 6, 2016

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

@SimonSapin
Copy link
Contributor Author

Rebased on top of #32020 which stabilized these methods. The libs team may want to discuss the (re)addition of the OwnedAsciiExt trait so this leaves it unstable for now, effectively reverting #32020.

Implementing `AsciiExt` for `String` and `Vec<u8>` caused a regression:
rust-lang#32074
and the `where Self: Sized` hack to have the `into_` methods in that trait
(which is also implemented for DST `str` and `[u8]`) was rather clunky.

CC rust-lang#27809
@alexcrichton
Copy link
Member

I feel like our story here is going off the rails, tagging this with T-libs to discuss during triage.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 6, 2016
@SimonSapin
Copy link
Contributor Author

#32074 should be fixed somehow for 1.8. some_string.eq_ignore_ascii_case(some_str) is not a niche case at all.

@alexcrichton
Copy link
Member

The libs team discussed this during triage today and the decision was to revert the into_ascii_* methods and go back to the state on 1.7 stable today. I'll send a PR soon (unless @SimonSapin beats me to it) to apply this revert.

As a result, though, I'm going to close this PR for now. We figured that we could continue discussion (including this course of action) on the tracking issue!

@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 17, 2016
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 17, 2016
The addition of these methods in rust-lang#31335 required adding impls of the trait for
the `String` and `Vec<T>` types. This unfortunately caused a regression (rust-lang#32074)
in type inference for using these methods which the libs team has decided to not
push forward with. These methods were stabilized in rust-lang#32020 which was intended to
get backported to beta, but the backport hasn't happened just yet. This commit
reverts both the addition and stabilization of these methods.

One proposed method of handling this, in rust-lang#32076, was to move the methods to an
extra trait to avoid conflicts with type inference. After some discussion,
however, the libs team concluded that we probably want to reevaluate what we're
doing here, so discussion will continue on the tracking issue, rust-lang#27809.
bors added a commit that referenced this pull request Mar 19, 2016
std: Revert addition of `into_ascii_*` methods

The addition of these methods in #31335 required adding impls of the trait for
the `String` and `Vec<T>` types. This unfortunately caused a regression (#32074)
in type inference for using these methods which the libs team has decided to not
push forward with. These methods were stabilized in #32020 which was intended to
get backported to beta, but the backport hasn't happened just yet. This commit
reverts both the addition and stabilization of these methods.

One proposed method of handling this, in #32076, was to move the methods to an
extra trait to avoid conflicts with type inference. After some discussion,
however, the libs team concluded that we probably want to reevaluate what we're
doing here, so discussion will continue on the tracking issue, #27809.

Closes #32074
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 24, 2016
The addition of these methods in rust-lang#31335 required adding impls of the trait for
the `String` and `Vec<T>` types. This unfortunately caused a regression (rust-lang#32074)
in type inference for using these methods which the libs team has decided to not
push forward with. These methods were stabilized in rust-lang#32020 which was intended to
get backported to beta, but the backport hasn't happened just yet. This commit
reverts both the addition and stabilization of these methods.

One proposed method of handling this, in rust-lang#32076, was to move the methods to an
extra trait to avoid conflicts with type inference. After some discussion,
however, the libs team concluded that we probably want to reevaluate what we're
doing here, so discussion will continue on the tracking issue, rust-lang#27809.

Conflicts:
	src/libstd/ascii.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants