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

use char instead of &str for single char patterns #69481

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

matthiaskrgr
Copy link
Member

No description provided.

Copy link
Contributor

@ecstatic-morse ecstatic-morse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after removing unnecessary escapes.

src/librustc_builtin_macros/format.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/demand.rs Outdated Show resolved Hide resolved
ecstatic-morse
ecstatic-morse approved these changes Feb 26, 2020
@ecstatic-morse
Copy link
Contributor

@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Feb 26, 2020

📌 Commit 8ceb9096ac72bab8b5566f22645c16e84634603f has been approved by ecstatic-morse

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 26, 2020
@Dylan-DPC-zz
Copy link

failed in rollup
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 26, 2020
bors added a commit that referenced this pull request Feb 27, 2020
Rollup of 4 pull requests

Successful merges:

 - #69439 (resolve: `lifetimes.rs` -> `late/lifetimes.rs`)
 - #69473 (update llvm to silence gcc 9 warnings)
 - #69479 (clarify operator precedence)
 - #69480 (Clean up E0373 explanation)

Failed merges:

 - #69481 (use char instead of &str for single char patterns)
 - #69496 (use find(x) instead of filter(x).next())

r? @ghost
@petrochenkov
Copy link
Contributor

#41993 was fixed, but I still think this replacement is ideologically wrong.
All these places don't care about unicode code points, they do substring search, it's just the substring happens to be pretty short.
char is a pretty rare data type that shouldn't be used unless you want unicode code points specifically.

@matthiaskrgr
Copy link
Member Author

When running these benchmarks that I found via the discussion you linked

#![feature(test)]

extern crate test;
use crate::test::black_box;
use crate::test::Bencher;

fn main() {
    println!("Hello, world!");
}

#[bench]
fn starts_with_char(b: &mut Bencher) {
    let text = black_box("kdjsfhlakfhlsghlkvcnljknfqiunvcijqenwodind");
    b.iter(|| {
        for _ in 0..1024 {
            black_box(text.starts_with('k'));
        }
    })
}

#[bench]
fn starts_with_str(b: &mut Bencher) {
    let text = black_box("kdjsfhlakfhlsghlkvcnljknfqiunvcijqenwodind");
    b.iter(|| {
        for _ in 0..1024 {
            black_box(text.starts_with("k"));
        }
    })
}

#[bench]
fn ends_with_char(b: &mut Bencher) {
    let text = black_box("kdjsfhlakfhlsghlkvcnljknfqiunvcijqenwodind");
    b.iter(|| {
        for _ in 0..1024 {
            black_box(text.ends_with('k'));
        }
    })
}

#[bench]
fn ends_with_str(b: &mut Bencher) {
    let text = black_box("kdjsfhlakfhlsghlkvcnljknfqiunvcijqenwodind");
    b.iter(|| {
        for _ in 0..1024 {
            black_box(text.ends_with("k"));
        }
    })
}

, char is consistently faster for me with default optimization levels:

running 4 tests
test ends_with_char   ... bench:         465 ns/iter (+/- 31)
test ends_with_str    ... bench:         544 ns/iter (+/- 79)
test starts_with_char ... bench:         356 ns/iter (+/- 38)
test starts_with_str  ... bench:         691 ns/iter (+/- 10)
running 4 tests
test ends_with_char   ... bench:         464 ns/iter (+/- 10)
test ends_with_str    ... bench:         544 ns/iter (+/- 15)
test starts_with_char ... bench:         355 ns/iter (+/- 11)
test starts_with_str  ... bench:         692 ns/iter (+/- 11)
running 4 tests
test ends_with_char   ... bench:         448 ns/iter (+/- 11)
test ends_with_str    ... bench:         527 ns/iter (+/- 2)
test starts_with_char ... bench:         340 ns/iter (+/- 33)
test starts_with_str  ... bench:         670 ns/iter (+/- 8)

@petrochenkov
Copy link
Contributor

If this is a perf improvement now, rather than just a stylistic change, then it should be ok.

@bors
Copy link
Contributor

bors commented Feb 27, 2020

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

@matthiaskrgr
Copy link
Member Author

rebased

@matthiaskrgr
Copy link
Member Author

r? @petrochenkov or @ecstatic-morse

@Dylan-DPC-zz
Copy link

@bors r=ecstatic-morse

@bors
Copy link
Contributor

bors commented Feb 28, 2020

📌 Commit 7c84ba1 has been approved by ecstatic-morse

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2020
bors added a commit that referenced this pull request Feb 28, 2020
Rollup of 10 pull requests

Successful merges:

 - #68989 (Update RELEASES.md for 1.42.0)
 - #69340 (instantiate_value_path: on `SelfCtor`, avoid unconstrained tyvars)
 - #69384 (parser: `token` -> `normalized_token`, `nonnormalized_token` -> `token`)
 - #69452 (typeck: use `Pattern` obligation cause more for better diagnostics)
 - #69481 (use char instead of &str for single char patterns)
 - #69522 (error_derive_forbidden_on_non_adt: be more graceful)
 - #69538 (Stabilize `boxed_slice_try_from`)
 - #69539 (late resolve, visit_fn: bail early if there's no body.)
 - #69541 (Remove unneeded calls to format!())
 - #69547 (remove redundant clones, references to operands, explicit boolean comparisons and filter(x).next() calls.)

Failed merges:

r? @ghost
@bors bors merged commit 07d9ed2 into rust-lang:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants