Lintcheck maintenance #10445
Lintcheck maintenance #10445
Conversation
|
r? @llogiq (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
lintcheck/src/config.rs
Outdated
| // look at the --threads arg, if 0 is passed, use the threads count | ||
| if config.max_jobs == 0 { | ||
| // automatic choice | ||
| config.max_jobs = num_cpus::get(); |
There was a problem hiding this comment.
Why not use std::thread::available_parallelism() here and avoid an extra dependency?
There was a problem hiding this comment.
Sure. I introduced the dependency in the first commit to keep the same semantics as was there before so that commits were self contained, and didn't think twice about removing it. Done, now in two commits.
samueltardieu
force-pushed
the
lintcheck-maintenance
branch
from
151ef5a to
f82c64a
Compare
Using `rayon::current_num_threads()` causes a bug:
```
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value:
ThreadPoolBuildError { kind: GlobalPoolAlreadyInitialized }',
src/main.rs:632:10
```
Moreover, using the number of threads and dividing it by 2 wouldn't
return the number of physical threads on modern processors which have
a varying number of threads per core.
It makes little sense to restrict ourselves to physical threads,
especially when, in modern architectures, cores with multiple threads
are often faster (performance) while cores with a unique threads are
often slower (efficient). The Rust runtime will make a better choice.
samueltardieu
force-pushed
the
lintcheck-maintenance
branch
from
f82c64a to
00dedfa
Compare
lintcheck/src/config.rs
Outdated
| // look at the --threads arg, if 0 is passed, use the threads count | ||
| if config.max_jobs == 0 { | ||
| // automatic choice | ||
| config.max_jobs = std::thread::available_parallelism().map(|n| n.get()).unwrap_or(1); |
There was a problem hiding this comment.
| config.max_jobs = std::thread::available_parallelism().map(|n| n.get()).unwrap_or(1); | |
| config.max_jobs = std::thread::available_parallelism().map_or(1, |n| n.get()); |
There was a problem hiding this comment.
Done. There should be a lint for that.
Neat GitHub interface btw, I didn't know this one.
This makes the code shorter and clearer. The only incompatible change is that an explicit command-line argument `--crates-toml=` will take precedence over the `LINTCHECK_TOML` environment variable.
samueltardieu
force-pushed
the
lintcheck-maintenance
branch
from
00dedfa to
79829d8
Compare
|
Thank you! And I think there is a lint for that, I'm curious why it didn't trigger during dogfood. @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Make
cargo lintcheck -j 0use all threads instead of panicking, and cleanup (and shorten) lintcheck's parsing code.changelog: none