-
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 u32 over Option<u32> in DebugLoc #82285
Conversation
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit a5bfabf2084e207703fee5aae82a5be5f4c8a759 with merge 0b2cff598b5954727775051cc41563a0ecf6119f... |
☀️ Try build successful - checks-actions |
Queued 0b2cff598b5954727775051cc41563a0ecf6119f with parent 9b471a3, future comparison URL. |
Finished benchmarking try commit (0b2cff598b5954727775051cc41563a0ecf6119f): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
📌 Commit a5bfabf2084e207703fee5aae82a5be5f4c8a759 has been approved by |
@bors r- Please squash (some of) the commits together. |
@rustbot label -S-waiting-on-author +S-waiting-on-review |
@bors r+ Sorry for not getting back to this sooner! |
📌 Commit 408d402 has been approved by |
☀️ Test successful - checks-actions |
ChangesOption<u32>
fields inDebugLoc
toOption<NonZeroU32>
. Since the respective fields (line
andcol
) are guaranteed to be 1-based, this layout optimization is a freebie.EDIT: Changes
Option<u32>
fields inDebugLoc
tou32
. As @bugadani pointed out, anOption<NonZeroU32>
is probably an unnecessary layer of abstraction since theNone
variant is always used asUNKNOWN_LINE_NUMBER
(which is just0
). Also,SourceInfo
inmetadata.rs
already uses au32
instead of anOption<u32>
to encode the same information, so I think this change is warranted.Since @jyn514 raised some concerns over measuring performance in a similar PR (#82255), does this need a perf run?