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.

Sign up for GitHub

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

Jump to bottom

Call module_name_to_str instead of just unwrapping #130680

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Call module_name_to_str instead of just unwrapping #130680

merged 1 commit into from
Sep 23, 2024

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Sep 21, 2024 •
edited
Loading

This makes the ICE message in #130678 more clear. It looks like not calling this function was just an oversight in #76859, but clearly not a major one because it's taken us 4 years to notice.

try-job: i686-msvc

@rustbot
Copy link
Collaborator

rustbot commented Sep 21, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 21, 2024
@saethlin saethlin mentioned this pull request Sep 21, 2024
Open
@jieyouxu jieyouxu assigned jieyouxu and unassigned cjgillot Sep 22, 2024
jieyouxu
jieyouxu approved these changes Sep 22, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks!

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 22, 2024

📌 Commit 6419aeb has been approved by jieyouxu

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 Sep 22, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 22, 2024
@GuillaumeGomez
Rollup merge of rust-lang#130680 - saethlin:module-name-to-str, r=jie…
f4509dc 
…youxu

Call module_name_to_str instead of just unwrapping

This makes the ICE message in rust-lang#130678 more clear. It looks like not calling this function was just an oversight in rust-lang#76859, but clearly not a major one because it's taken us 4 years to notice.
@GuillaumeGomez GuillaumeGomez mentioned this pull request Sep 22, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2024
@bors
Auto merge of rust-lang#130695 - GuillaumeGomez:rollup-7w2zou6, r=Gui…
5242b6f 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#129545 (rustdoc: redesign toolbar and disclosure widgets)
 - rust-lang#130658 (Fix docs of compare_bytes)
 - rust-lang#130670 (delay uncapping the max_read_size in File::read_to_end)
 - rust-lang#130680 (Call module_name_to_str instead of just unwrapping)
 - rust-lang#130690 (interpret: remove outdated FIXME)

r? `@ghost`
`@rustbot` modify labels: rollup
@GuillaumeGomez
Copy link
Member

I wonder if this is the one which failed in #130695.

@saethlin
Copy link
Member Author

That's an exotic failure mode. Let's see!

@bors try

@bors
Copy link
Contributor

bors commented Sep 22, 2024

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

@saethlin
Copy link
Member Author

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 22, 2024
@saethlin
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2024
@bors
Auto merge of rust-lang#130680 - saethlin:module-name-to-str, r=<try>
b0f3400 
Call module_name_to_str instead of just unwrapping

This makes the ICE message in rust-lang#130678 more clear. It looks like not calling this function was just an oversight in rust-lang#76859, but clearly not a major one because it's taken us 4 years to notice.

try-job: i686-msvc
@bors
Copy link
Contributor

bors commented Sep 22, 2024

⌛ Trying commit 6419aeb with merge b0f3400...

@bors
Copy link
Contributor

bors commented Sep 22, 2024

☀️ Try build successful - checks-actions
Build commit: b0f3400 (b0f3400b7b7ef8b7c91aadf23ce3ba66c658f382)

@saethlin
Copy link
Member Author

Well. Having looked through the other PRs in that rollup I agree with @GuillaumeGomez that nothing else in there looks related. But also I cannot explain how this PR would corrupt a DenseMap, and we just passed a try build for the job that failed, so either this PR is fine or the failure is nondeterministic.

@bors r=jieyouxu rollup=never just in case this needs to be reverted or fails again

@bors
Copy link
Contributor

bors commented Sep 22, 2024

📌 Commit 6419aeb has been approved by jieyouxu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 22, 2024
@bors
Copy link
Contributor

bors commented Sep 22, 2024

⌛ Testing commit 6419aeb with merge d14c1c7...

@bors
Copy link
Contributor

bors commented Sep 23, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing d14c1c7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2024
@bors bors merged commit d14c1c7 into rust-lang:master Sep 23, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 23, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d14c1c7): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 0.3%, secondary -0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 3.1% [3.1%, 3.1%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -2.5% [-2.5%, -2.5%] 1
Improvements ✅
(secondary)		 -0.8% [-0.8%, -0.7%] 2
All ❌✅ (primary) 0.3% [-2.5%, 3.1%] 2

Cycles

Results (primary 0.5%, secondary 3.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.5% [0.4%, 0.5%] 3
Regressions ❌
(secondary)		 3.1% [2.0%, 4.2%] 15
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.5% [0.4%, 0.5%] 3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.792s -> 770.325s (0.07%)
Artifact size: 341.51 MiB -> 341.57 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
7 participants
@saethlin @rustbot @jieyouxu @bors @GuillaumeGomez @rust-timer @cjgillot