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 specialization of ToString for char #73465

Merged
merged 1 commit into from
Jun 20, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jun 18, 2020

Closes #73462

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2020
@Mark-Simulacrum
Copy link
Member

Hm, this is an interesting case where AFAIK, there's no way to know this impl exists other than performance. I'm inclined to say it doesn't need FCP then. @rust-lang/libs, what do you think?

@lzutao Have you checked that this fixes the associated issue? Or we're just assuming it should?

@sfackler
Copy link
Member

Is the prohibition on new specializations still in effect?

@Mark-Simulacrum
Copy link
Member

Cc @matthewjasper @nikomatsakis

I think this is just expanding an existing specialization-available trait so we probably don't need to worry too much?

@matthewjasper
Copy link
Contributor

I'm fine with this specialization being added. We use min_specialization everywhere apart from core, so I think that "we have some checks in place" for this.

@sfackler
Copy link
Member

r=me assuming there aren't any objections on the specialization.

@tesuji
Copy link
Contributor Author

tesuji commented Jun 18, 2020

Yes. The assembly code after this PR:

check_stage1::foo:
 push    rbx
 mov     rbx, rdi
 mov     edi, 1
 mov     esi, 1
 call    qword, ptr, [rip, +, __rust_alloc@GOTPCREL]
 test    rax, rax
 je      .LBB1_1
 mov     byte, ptr, [rax], 97
 mov     qword, ptr, [rbx], rax
 movaps  xmm0, xmmword, ptr, [rip, +, .LCPI1_0]
 movups  xmmword, ptr, [rbx, +, 8], xmm0
 mov     rax, rbx
 pop     rbx
 ret
.LBB1_1:
 mov     edi, 1
 mov     esi, 1
 call    alloc::raw_vec::RawVec<T,A>::allocate_in::{{closure}}
 ud2

where:

pub fn foo() -> String {
    'a'.to_string()
}

@SimonSapin
Copy link
Contributor

[97] is UTF-8 for a, so it looks like all of encode_utf8 was constant-folded in this example! With a char parameter instead of a constant the assembly code for String::from(c.encode_utf8(&mut [0; 4])) is longer, but still shorter than going through Display.

@Mark-Simulacrum
Copy link
Member

@bors r=sfackler per @matthewjasper's approval here. It also seems like we can easily revert this down the line if needed.

@bors
Copy link
Contributor

bors commented Jun 18, 2020

📌 Commit e5c5df8 has been approved by sfackler

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 18, 2020
@cuviper cuviper assigned sfackler and unassigned cuviper Jun 18, 2020
@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 19, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#71568 (Document unsafety in slice/sort.rs)
 - rust-lang#72709 (`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc)
 - rust-lang#73214 (Add asm!() support for hexagon)
 - rust-lang#73248 (save_analysis: improve handling of enum struct variant)
 - rust-lang#73257 (ty: projections in `transparent_newtype_field`)
 - rust-lang#73261 (Suggest `?Sized` when applicable for ADTs)
 - rust-lang#73300 (Implement crate-level-only lints checking.)
 - rust-lang#73334 (Note numeric literals that can never fit in an expected type)
 - rust-lang#73357 (Use `LocalDefId` for import IDs in trait map)
 - rust-lang#73364 (asm: Allow multiple template string arguments; interpret them as newline-separated)
 - rust-lang#73382 (Only display other method receiver candidates if they actually apply)
 - rust-lang#73465 (Add specialization of `ToString for char`)
 - rust-lang#73489 (Refactor hir::Place)

Failed merges:

r? @ghost
@bors bors merged commit d2272d4 into rust-lang:master Jun 20, 2020
@tesuji tesuji deleted the spec-char-tostring branch June 20, 2020 02:35
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bad performance of char literal to_string() vs str literal to_string()
9 participants