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

panic_bounds_check: use caller_location, like PanicFnLangItem #69850

Merged
merged 4 commits into from
Mar 11, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 9, 2020

The PanicFnLangItem got switched to using #[caller_location] at some point, but PanicBoundsCheckFnLangItem was kept in the old style. For consistency, switch that one over to use #[caller_location] as well.

This is also helpful for Miri as it means the assert_panic machine hook never needs to know the current Span.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(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 Mar 9, 2020
@Centril Centril added the F-track_caller `#![feature(track_caller)]` label Mar 9, 2020
@Centril
Copy link
Contributor

Centril commented Mar 9, 2020

r? @Centril

@bors try @rust-timer queue

r=me with clean perf

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@rust-highfive rust-highfive assigned Centril and unassigned KodrAus Mar 9, 2020
@bors
Copy link
Contributor

bors commented Mar 9, 2020

⌛ Trying commit 1a9fc18 with merge 568fa7c...

bors added a commit that referenced this pull request Mar 9, 2020
panic_bounds_check: use caller_location, like PanicFnLangItem

The `PanicFnLangItem` got switched to using `#[caller_location]` at some point, but `PanicBoundsCheckFnLangItem` was kept in the old style. For consistency, switch that one over to use `#[caller_location]` as well.

This is also helpful for Miri as it means the `assert_panic` machine hook never needs to know the current `Span`.
@Centril
Copy link
Contributor

Centril commented Mar 9, 2020

cc also @eddyb @anp

src/libcore/panicking.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Mar 9, 2020

☀️ Try build successful - checks-azure
Build commit: 568fa7c (568fa7cb76e8cfbf4e2491e846c1baf98ec7a356)

@rust-timer
Copy link
Collaborator

Queued 568fa7c with parent 2cb0b85, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 568fa7c, comparison URL.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 9, 2020

Perf looks like noise to me.
@eddyb I found a way to add #[track_caller] to panic_fmt as well. It required a bit more cfg(bootstrap) surgery.

@Centril
Copy link
Contributor

Centril commented Mar 10, 2020

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned Centril Mar 10, 2020
src/libcore/panicking.rs Outdated Show resolved Hide resolved
@eddyb
Copy link
Member

eddyb commented Mar 10, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 10, 2020

📌 Commit 0b2329d has been approved by eddyb

@bors
Copy link
Contributor

bors commented Mar 10, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@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 Mar 10, 2020
Copy link
Member

@anp anp left a comment

Choose a reason for hiding this comment

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

Nice!

bors added a commit that referenced this pull request Mar 11, 2020
Rollup of 8 pull requests

Successful merges:

 - #66472 (--show-coverage json)
 - #69603 (tidy: replace `make check` with `./x.py test` in documentation)
 - #69760 (Improve expression & attribute parsing)
 - #69828 (fix memory leak when vec::IntoIter panics during drop)
 - #69850 (panic_bounds_check: use caller_location, like PanicFnLangItem)
 - #69876 (Add long error explanation for E0739)
 - #69888 ([Miri] Use a session variable instead of checking for an env var always)
 - #69893 (librustc_codegen_llvm: Use slices instead of 0-terminated strings)

Failed merges:

r? @ghost
@bors bors merged commit 3853da7 into rust-lang:master Mar 11, 2020
@RalfJung RalfJung deleted the panic-bounds-check branch March 11, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-track_caller `#![feature(track_caller)]` 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.

8 participants