Skip to content

Conversation

@Kijewski
Copy link
Contributor

@Kijewski Kijewski commented Jan 26, 2025

This PR lets impl Default for Rc<str> re-use the implementation for Rc::<[u8]>::default(). The previous version only calculted the memory layout at runtime, even though it should be known at compile time, resulting in an additional function call.

The same optimization is done for Rc<CStr>.

Generated byte code: https://godbolt.org/z/dfq73jsoP.

Resolves #135784.

Cc @Billy-Sheppard.

@rustbot
Copy link
Collaborator

rustbot commented Jan 26, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 26, 2025
This PR lets `impl Default for Rc<str>` re-use the implementation
for `Rc::<[u8]>::default()`. The previous version only calculted the
memory layout at runtime, even though it should be known at compile
time, resulting in an additional function call.

The same optimization is done for `Rc<CStr>`.

Generated byte code: <https://godbolt.org/z/dfq73jsoP>.

Resolves <rust-lang#135784>.
@ibraheemdev
Copy link
Member

Approving this without a perf run because the codegen is clearly better (and a perf run is unlikely to provide meaningful results). Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 8, 2025

📌 Commit e090db8 has been approved by ibraheemdev

It is now in the queue for this repository.

@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 Feb 8, 2025
@mati865
Copy link
Member

mati865 commented Feb 8, 2025

FWIW, there are guidelines on how to write comments regarding the use of unsafe functions: https://std-dev-guide.rust-lang.org/policy/safety-comments.html

@Kijewski
Copy link
Contributor Author

Kijewski commented Feb 8, 2025

Normally I follow that guideline, but I adapted to what I found in the file I changed: No "SAFETY" keywords and short sentences (and most unsafe blocks have no comments at all). IMHO someone should give the file some love and adapt the code to current guidelines.

@mati865
Copy link
Member

mati865 commented Feb 8, 2025

Definitely there there are some SAFETY keywords in this file, although as you have noticed, they are not present everywhere they should be.

IMHO someone should give the file some love and adapt the code to current guidelines.

There were attempts in the past, like #63793, but as you can probably tell, this is not a trivial task. I wouldn't block this PR on that, but I hope to raise awareness for the future contributions.

@Kijewski
Copy link
Contributor Author

Kijewski commented Feb 8, 2025

Definitely there there are some SAFETY keywords in this file

Ah, I missed the 18 instances in a 4000+ lines file. But you are right, newer functions further down do follow the guideline. I'll do too on future contributions regardless if the rest of a file does not.

@bors bors merged commit 785a4eb into rust-lang:master Feb 8, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 8, 2025
@Kijewski Kijewski deleted the pr-rc-str-default branch February 9, 2025 00:58
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.