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.

Sign up for GitHub

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

Jump to bottom

core::ascii::escape_default: reduce struct size #89015

Merged
merged 1 commit into from
Sep 22, 2021
Merged

core::ascii::escape_default: reduce struct size #89015

merged 1 commit into from
Sep 22, 2021

Conversation

klensy
Copy link
Contributor

@klensy klensy commented Sep 16, 2021

No description provided.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

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

Is there some concrete motivation for this? Generally speaking I don't think the trade off of the unsafe code is worth it here; the escape_default iterator is unlikely to be in a particularly hot path. Further, it's not inlined today (and can't be without -Zbuild-std or equivalent), since all of this code is not generic and so is codegen'd into std. (This is not to say it should be inlined; I think this is fine).

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2021
@klensy
Copy link
Contributor Author

klensy commented Sep 17, 2021 •
edited
Loading

Motivation was that i looked around std and checked if there some needless bound checks or similar things (plus looking for function usage in compiler or deps).
Display implementation already uses unsafe, but i can drop second commit, anyway.

Btw, can i replace things like this collect-via-iterator with to_string in this PR?

rust/compiler/rustc_ast/src/util/literal.rs

Lines 170 to 172 in e0c38af

LitKind::Byte(byte) => {
let string: String = ascii::escape_default(byte).map(Into::<char>::into).collect();
(token::Byte, Symbol::intern(&string), None)

@Mark-Simulacrum
Copy link
Member

Motivation was that i looked around std and checked if there some needless bound checks or similar things (plus looking for function usage in compiler or deps).

I don't feel like this is sufficient justification for introducing unsafe code here. While it is not too hard to justify soundness here, it doesn't seem warranted to me, since it makes the code harder to read and edit and for little benefit. Bounds checking is not that expensive.

Making the struct smaller also seems unlikely to be of much help, though I'm not really opposed. Practically speaking it'll almost always be put on the stack, and there it doesn't really matter that much whether we are using 24 or 6 bytes -- it's a bump allocation anyway. But I'm OK r+'ing the first commit here.

Btw, can i replace things like this collect-via-iterator with to_string in this PR?

This PR does not seem like the right place for changes to the compiler, no. Not sure I follow your question entirely, but that feels like a change best left for separate PR(s).

Mark-Simulacrum
Mark-Simulacrum reviewed Sep 18, 2021
View reviewed changes
library/core/src/ascii.rs Outdated
@@ -138,7 +138,7 @@ impl FusedIterator for EscapeDefault {}
impl fmt::Display for EscapeDefault {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// SAFETY: ok because `escape_default` created only valid utf-8 data
f.write_str(unsafe { from_utf8_unchecked(&self.data[self.range.clone()]) })
f.write_str(unsafe { from_utf8_unchecked(&self.data[..self.range.end as usize]) })
Copy link
Member

Choose a reason for hiding this comment

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

This is a change in behavior if the iterator has already been advanced from the front, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, when dropped second commit noticed, that i changed this in first.

@klensy klensy changed the title core::ascii::escape_default: reduce struct size and number of bound checks core::ascii::escape_default: reduce struct size Sep 18, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@klensy
Copy link
Contributor Author

klensy commented Sep 18, 2021

@rustbot label: -S-waiting-on-author +S-waiting-on-review

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 20, 2021

📌 Commit cccd6e0 has been approved by Mark-Simulacrum

@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 Sep 20, 2021
@the8472 the8472 mentioned this pull request Sep 21, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2021
@bors
Auto merge of rust-lang#89158 - the8472:rollup-3e4ijth, r=the8472
d8d1d10 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#88795 (Print a note if a character literal contains a variation selector)
 - rust-lang#89015 (core::ascii::escape_default: reduce struct size)
 - rust-lang#89078 (Cleanup: Remove needless reference in ParentHirIterator)
 - rust-lang#89086 (Stabilize `Iterator::map_while`)
 - rust-lang#89096 ([bootstrap] Improve the error message when `ninja` is not found to link to installation instructions)
 - rust-lang#89113 (dont `.ensure()` the `thir_abstract_const` query call in `mir_build`)
 - rust-lang#89114 (Fixes a technicality regarding the size of C's `char` type)
 - rust-lang#89115 (:arrow_up: rust-analyzer)
 - rust-lang#89126 (Fix ICE when `indirect_structural_match` is allowed)
 - rust-lang#89141 (Impl `Error` for `FromSecsError` without foreign type)
 - rust-lang#89142 (Fix match for placeholder region)
 - rust-lang#89147 (add case for checking const refs in check_const_value_eq)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 051168b into rust-lang:master Sep 22, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 22, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 28, 2021
@bors
Auto merge of rust-lang#89158 - the8472:rollup-3e4ijth, r=the8472
b51b172 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#88795 (Print a note if a character literal contains a variation selector)
 - rust-lang#89015 (core::ascii::escape_default: reduce struct size)
 - rust-lang#89078 (Cleanup: Remove needless reference in ParentHirIterator)
 - rust-lang#89086 (Stabilize `Iterator::map_while`)
 - rust-lang#89096 ([bootstrap] Improve the error message when `ninja` is not found to link to installation instructions)
 - rust-lang#89113 (dont `.ensure()` the `thir_abstract_const` query call in `mir_build`)
 - rust-lang#89114 (Fixes a technicality regarding the size of C's `char` type)
 - rust-lang#89115 (:arrow_up: rust-analyzer)
 - rust-lang#89126 (Fix ICE when `indirect_structural_match` is allowed)
 - rust-lang#89141 (Impl `Error` for `FromSecsError` without foreign type)
 - rust-lang#89142 (Fix match for placeholder region)
 - rust-lang#89147 (add case for checking const refs in check_const_value_eq)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.
6 participants
@klensy @rust-highfive @Mark-Simulacrum @rust-log-analyzer @bors @rustbot