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 box syntax everywhere #87804

Closed
wants to merge 8 commits into from
Closed

Conversation

est31
Copy link
Member

@est31 est31 commented Aug 5, 2021

Alternative PR to #87781 .

As suggested by @leonardo-m :#87781 (comment)

Result of running:

rg -l "Box::new" compiler/ | rg '.rs$' | rg -v 'cranelift' | xargs sed -i 's/Box::new(/box (/'
@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Aug 5, 2021
@est31
Copy link
Member Author

est31 commented Aug 5, 2021

@bors try

@bors
Copy link
Contributor

bors commented Aug 5, 2021

⌛ Trying commit cbd8fcd with merge 69c94e269ddbe56fb5388e19bcd469b6500fa5ea...

@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Aug 6, 2021

⌛ Trying commit d5d2fb6 with merge 97d3a002940f685cc3fc98436ceaaa36eb1fb802...

@bors
Copy link
Contributor

bors commented Aug 6, 2021

☀️ Try build successful - checks-actions
Build commit: 97d3a002940f685cc3fc98436ceaaa36eb1fb802 (97d3a002940f685cc3fc98436ceaaa36eb1fb802)

@rust-timer
Copy link
Collaborator

Queued 97d3a002940f685cc3fc98436ceaaa36eb1fb802 with parent 1f94abc, future comparison URL.

@@ -49,7 +49,7 @@ macro_rules! err_exhaust {
#[macro_export]
macro_rules! err_machine_stop {
($($tt:tt)*) => {
$crate::mir::interpret::InterpError::MachineStop(Box::new($($tt)*))
$crate::mir::interpret::InterpError::MachineStop(box ($($tt)*))
Copy link
Member

Choose a reason for hiding this comment

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

This will mean other crates using the macro (such as standalone Miri) need to also enable the box feature, right? I think I'd prefer to avoid that.

Copy link
Member

Choose a reason for hiding this comment

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

There exists #[allow_internal_unstable] to prevent that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Example macro that does this for box syntax is the vec macro. As @petrochenkov correctly assumed, this PR was a test run though to explore the implications of using box syntax everywhere.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (97d3a002940f685cc3fc98436ceaaa36eb1fb802): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 6, 2021
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2021
@bjorn3
Copy link
Member

bjorn3 commented Aug 6, 2021

This is a slight regression of up to 0.3%. No improvements.

@petrochenkov
Copy link
Contributor

I assume this was opened only to measure performance, without intent to actually merge the changes, so closing.

@est31 est31 mentioned this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants