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

Rollup of 8 pull requests #122117

Merged
merged 17 commits into from
Mar 7, 2024
Merged

Rollup of 8 pull requests #122117

merged 17 commits into from
Mar 7, 2024

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

workingjubilee and others added 17 commits March 5, 2024 20:52
Clarify the FatalErrorHandler API that we use:
- Identify rustc's LLVM ERRORs by prefixing them
- Comment heavily on its interior, while we are here
C++ style guides I am aware of recommend specifically preferring = syntax
for any classes with fairly obvious constructors[^0] that do not perform
any complicated logic in their constructor. I contend that all constructors
that the `rustc_llvm` code uses qualify. This has only become more common
since C++ 17 guaranteed many cases of copy initialization elision.

The other detail is that I tried to ask another contributor with
infinitely more C++ experience than me (i.e. any) what this constructor
syntax was, and they thought it was a macro. I know of no other language
that has adopted this same syntax. As the rustc codebase features many
contributors experienced in many other languages, using a less...
unique... style has many other benefits in making this code more
lucid and maintainable, which is something it direly needs.

[^0]: e.g. https://abseil.io/tips/88
As the FIXME itself notes, there's nothing to fix here.
This commit is extracted from rust-lang#122036 and adds a new directive to the
`compiletest` test runner, `//@ needs-threads`. This is intended to
capture the need that a target must implement threading to execute a
specific test, typically one that uses `std::thread`. This is primarily
done for WebAssembly targets which currently do not have threads by
default. This enables transitioning a lot of `//@ ignore-wasm*`-style
ignores into a more self-documenting `//@ needs-threads` directive.
Additionally the `wasm32-wasi-preview1-threads` target, for example,
does actually have threads, but isn't tested in CI at this time. This
change enables running these tests for that target, but not other wasm
targets.
Add better explanation for `rustc_index::IndexVec`

I feel like I didn't do a great job explaining what this does in rust-lang#119800, so this PR tries to give an example of why and how you would use it.

Addresses rust-lang#93792.
…r=cuviper

Clarify FatalErrorHandler

- Identify rustc's LLVM ERRORs by prefixing them
- Comment heavily on its interior, while we are here
… r=cuviper

Explicitly assign constructed C++ classes

C++ style guides I am aware of recommend specifically preferring = syntax for any classes with fairly obvious constructors[^0] that do not perform any complicated logic in their constructor. I contend that all constructors that the `rustc_llvm` code uses qualify. This has only become more common since C++ 17 guaranteed many cases of copy initialization elision.

The other detail is that I tried to ask another contributor with infinitely more C++ experience than me (i.e. any) what this constructor syntax was, and they thought it was a macro. I know of no other language that has adopted this same syntax. As the rustc codebase features many contributors experienced in many other languages, using a less... unique... style has many other benefits in making this code more lucid and maintainable, which is something it direly needs.

[^0]: e.g. https://abseil.io/tips/88
Refer to "slice" instead of "vector" in Ord and PartialOrd trait impl of slices

The trait implementation comments of Ord and PartialOrd for slice incorrectly mention "vectors" instead of "slices".
This PR fixes those two comments as requested in rust-lang#122071.
Remove unnecessary fixme on new thread stack size

As the FIXME itself notes, there's nothing to fix here.

And as the documentation for [`CreateThread`] says of `dwStackSize`, the value is rounded up to the nearest page. A 4kb stack is very small but perfectly usable if you're careful. Of course it will be very limited but there's no reason to add artificial limits. We don't know what the user is doing.

[`CreateThread`]: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createthread
…, r=workingjubilee

Remove outdated footnote "missing-stack-probe" in platform-support

... after rust-lang#120055 and rust-lang#118491.

cc rust-lang#77071 (comment).
…eLapkin

Temporarily make allow-by-default the `non_local_definitions` lint

T-lang [decided in their triage meeting](https://hackmd.io/U-CKiZx_RKiaANAPXtWf7g#non_local_definitions-common-issues-impl-for-ampLocal-FromltLocalgt-for-Global-%E2%80%A6-rust121621) to try to use a [better logic](rust-lang#121621 (comment)) for detecting non-local `impl` definitions given the [numerous reports](rust-lang#121621) we got.

Until that is done and also because the beta cut is next week, switch the lint to allow-by-default until it's implemented.

r? `@WaffleLapkin`
…reads, r=workingjubilee

compiletest: Add a `//@ needs-threads` directive

This commit is extracted from rust-lang#122036 and adds a new directive to the `compiletest` test runner, `//@ needs-threads`. This is intended to capture the need that a target must implement threading to execute a specific test, typically one that uses `std::thread`. This is primarily done for WebAssembly targets which currently do not have threads by default. This enables transitioning a lot of `//@ ignore-wasm*`-style ignores into a more self-documenting `//@ needs-threads` directive. Additionally the `wasm32-wasi-preview1-threads` target, for example, does actually have threads, but isn't tested in CI at this time. This change enables running these tests for that target, but not other wasm targets.
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Mar 6, 2024
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=8

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit 5642b04 has been approved by matthiaskrgr

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 Mar 6, 2024
@bors
Copy link
Contributor

bors commented Mar 7, 2024

⌛ Testing commit 5642b04 with merge d03b986...

@bors
Copy link
Contributor

bors commented Mar 7, 2024

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing d03b986 to master...

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

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#122015 Add better explanation for rustc_index::IndexVec 9c2a5ed95d41f7c11f2367c968a7e9b7395129cb (link)
#122061 Clarify FatalErrorHandler 83259f6312266a3c2e2411c3381c4ffe5f5c4208 (link)
#122062 Explicitly assign constructed C++ classes 79698587fb9271adbd754a0377c6d68531bc50bc (link)
#122072 Refer to "slice" instead of "vector" in Ord and PartialOrd … 98078d62583fb43e8c4e7831f1e139f4c08e4a0a (link)
#122088 Remove unnecessary fixme on new thread stack size ee191741e924e4eb48204f92da587a17fc15d6d4 (link)
#122094 Remove outdated footnote "missing-stack-probe" in platform-… d144b8d44f1f193ec25eb50f5ebae5e3eb74138d (link)
#122107 Temporarily make allow-by-default the `non_local_definition… 704eb8c2a2ec0715d842532a2dee8ed266737950 (link)
#122109 compiletest: Add a //@ needs-threads directive a1dc5d8ddb8bf6c4f884ae4bb65b8a15a1278a9c (link)

previous master: 7d3702e472

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d03b986): comparison URL.

Overall result: ✅ improvements - 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
Improvements ✅
(primary)
-2.1% [-3.9%, -0.4%] 12
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.1% [-3.9%, -0.4%] 12

Max RSS (memory usage)

Results

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
Regressions ❌
(secondary)
2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)
-1.1% [-1.3%, -1.0%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.1% [-1.3%, -1.0%] 4

Cycles

Results

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
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
-2.3% [-2.5%, -1.9%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.3% [-2.5%, -1.9%] 6

Binary size

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

Bootstrap: 647.359s -> 646.949s (-0.06%)
Artifact size: 175.04 MiB -> 174.91 MiB (-0.07%)

@matthiaskrgr matthiaskrgr deleted the rollup-3yrv3j6 branch March 16, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants