-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 NonZeroU32
in newtype_index!
macro, change syntax
#53315
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me, unless you feel comments need addressing.
src/librustc_driver/test.rs
Outdated
|
||
fn d2() -> ty::DebruijnIndex { | ||
ty::INNERMOST.shifted_in(1) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because the get
method of NonZeroU32
is not a const fn
, so we cannot evaluate shifted_in
as a const method anymore.
@bors r=Mark-Simulacrum |
📌 Commit fa86a854e679ff9833715ae5968f200a5d3ce09f has been approved by |
|
@bors try (let's do some benchmarking here first) |
⌛ Trying commit fa86a854e679ff9833715ae5968f200a5d3ce09f with merge 850f704f57dbfefaf6714cedb1a0d880529b9a5a... |
☀️ Test successful - status-travis |
@rust-timer build 850f704f57dbfefaf6714cedb1a0d880529b9a5a |
Insufficient permissions to issue commands to rust-timer. |
@rust-timer build 850f704f57dbfefaf6714cedb1a0d880529b9a5a |
Success: Queued 850f704f57dbfefaf6714cedb1a0d880529b9a5a with parent 3e05f80, comparison URL. |
@eddyb profiling sounds good, yes |
Perf says this is pretty painful. I wonder how simply not using the 0th slot in the various indexmaps would compare, instead of adding and subtracting 1. The searchability and ergro stuff is good though. |
Interesting. I'm quite surprised, actually. I can certainly roll back the |
That said, it seems like it's mostly a wash -- except for keccak, which is considerably slower (I wonder what makes it quite so different?). I've noticed the ctfe tests fluctuating 1-2% elsewhere. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc_mir/lib.rs
Outdated
@@ -28,6 +28,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment! | |||
#![feature(decl_macro)] | |||
#![cfg_attr(stage0, feature(macro_vis_matcher))] | |||
#![feature(exhaustive_patterns)] | |||
#![feature(const_fn)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min_const_fn
should suffice I think?
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
]; | ||
|
||
unsafe { | ||
$type::from_u32_unchecked(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't possible with min const fn. needs the const fn feature gate in this crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do the struct init directly without the unsafe fn call
☔ The latest upstream changes (presumably #52626) made this pull request unmergeable. Please resolve the merge conflicts. |
Also, adjust the MAX to be `u32::MAX - 1`, leaving room for `u32::MAX` to become a sentinel value in the future.
This reverts (part of) commit cb9a336ae2cf6a75fdcc130853286349cb424c96.
f19a7a4
to
ab43c1e
Compare
@bors r=Mark-Simulacrum |
📌 Commit ab43c1e has been approved by |
…Simulacrum use `NonZeroU32` in `newtype_index!`macro, change syntax Various improvements to the `newtype_index!` macro: - Use `NonZeroU32` so that `Option<T>` is cheap - More ergonomic helper method, no need to import `Idx` trait all the time - Improve syntax to use `struct` keyword so that ripgrep works to find type def'n Fixes rust-lang#50337 I'm curious to see if this passes tests =)
Rollup of 10 pull requests Successful merges: - #53315 (use `NonZeroU32` in `newtype_index!`macro, change syntax) - #53932 ([NLL] Remove base_place) - #53942 (Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.) - #53973 (Have rust-lldb look for the rust-enabled lldb) - #53981 (Implement initializer() for FileDesc) - #53987 (rustbuild: allow configuring llvm version suffix) - #53993 (rustc_resolve: don't record uniform_paths canaries as reexports.) - #54007 (crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint) - #54040 (update books for next release) - #54050 (Update `petgraph` dependency to 0.4.13 to fix build with nightly)
👏 👏 👏 |
Various improvements to the
newtype_index!
macro:NonZeroU32
so thatOption<T>
is cheapIdx
trait all the timestruct
keyword so that ripgrep works to find type def'nFixes #50337
I'm curious to see if this passes tests =)
r? @oli-obk (picked because of reviewer suggestions...)