-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Change char::eq_ignore_ascii_case
to take a Borrow<char>
instead of &char
#57228
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
Change char::eq_ignore_ascii_case
to take a Borrow<char>
instead of &char
#57228
Conversation
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Personally, I'd rather see these deprecated entirely for |
@clarcharr |
...right, I don't know how I messed that up. I'm still not super fond of this method and wonder if there's a more ergonomic way to do this. |
I'm unclear on how we deal with this backwards compatibility aspect. For example, this shows a form of code that would fail with the proposed changes: fn before(_: &char, _: &char) -> bool {
false
}
fn after(_: &char, _: impl std::borrow::Borrow<char>) -> bool {
false
}
fn example(_: fn(&char, &char) -> bool) {}
fn main() {
example(before);
example(after);
} r? @BurntSushi |
Nice catch @shepmaster . I was hoping Rust would automatically deduce the types. I think for this reason I don't think this PR should be merged. I'm going to work on making tests for these things. I need to get a real development environment set up first. |
Ping from triage @czipperz: What is the status of this PR? Should it be closed for now? Or do you think you'll be able to work on fixing the issue? |
Yeah shoot I forget to close this @TimNN |
Resolves #57227