Skip to content

Conversation

tshepang
Copy link
Member

No description provided.

@rust-highfive
Copy link
Contributor

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2020
@tshepang tshepang changed the title make cncurrency helper more pleasant to read make concurrency helper more pleasant to read Oct 18, 2020
Comment on lines 7 to 9
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous version looks a bit easier to read on this part since it specifically mention usize.

Also, I wonder why it needs to check n > 0 here?

I wonder will it be better to parse to NonZeroUsize so the guarantees are already there.

Suggested change
match value.parse().ok() {
Some(n) if n > 0 => n,
_ => panic!("RUST_TEST_THREADS is `{}`, should be a positive integer.", value),
match value.parse::<std::num::NonZeroUsize>().ok() {
Some(n) => n.get(),
_ => panic!("RUST_TEST_THREADS is `{}`, should be a positive integer.", value),

Copy link
Member Author

Choose a reason for hiding this comment

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

type inference/coercion is helped by the return type, or do you feel it's too far, preferring the type spelled out close to the declaration

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Looks like this failed to build. https://github.com/rust-lang/rust/pull/78065/checks?check_run_id=1270956557

    Checking test v0.0.0 (/checkout/library/test)
error[E0282]: type annotations needed
 --> library/test/src/helpers/concurrency.rs:8:24
  |
8 |             Some(n) if n.is_positive() => n,
  |                        ^ cannot infer type
  |
  = note: type must be known at this point
help: consider specifying the type argument in the method call
  |
7 |         match value.parse::<F>().ok() {
  |                          ^^^^^

error: aborting due to previous error

@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 20, 2020
@pickfire
Copy link
Contributor

@dtolnay How do you bold the code blocks?

@tshepang tshepang closed this Oct 21, 2020
@tshepang tshepang reopened this Oct 21, 2020
@tshepang tshepang requested a review from dtolnay October 21, 2020 07:38
@tshepang
Copy link
Member Author

tshepang commented Nov 4, 2020

build failure fixed @dtolnay

@dtolnay
Copy link
Member

dtolnay commented Nov 6, 2020

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 6, 2020

📌 Commit 628fb9f has been approved by dtolnay

@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 Nov 6, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 7, 2020
make concurrency helper more pleasant to read
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 7, 2020
make concurrency helper more pleasant to read
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 7, 2020
make concurrency helper more pleasant to read
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 7, 2020
make concurrency helper more pleasant to read
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
make concurrency helper more pleasant to read
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2020
Rollup of 19 pull requests

Successful merges:

 - rust-lang#76097 (Stabilize hint::spin_loop)
 - rust-lang#76227 (Stabilize `Poll::is_ready` and `is_pending` as const)
 - rust-lang#78065 (make concurrency helper more pleasant to read)
 - rust-lang#78570 (Remove FIXME comment in print_type_sizes ui test suite)
 - rust-lang#78572 (Use SOCK_CLOEXEC and accept4() on more platforms.)
 - rust-lang#78658 (Add a tool to run `x.py` from any subdirectory)
 - rust-lang#78706 (Fix run-make tests running when LLVM is disabled)
 - rust-lang#78728 (Constantify `UnsafeCell::into_inner` and related)
 - rust-lang#78775 (Bump Rustfmt and RLS)
 - rust-lang#78788 (Correct unsigned equivalent of isize to be usize)
 - rust-lang#78811 (Make some std::io functions `const`)
 - rust-lang#78828 (use single char patterns for split() (clippy::single_char_pattern))
 - rust-lang#78841 (Small cleanup in `TypeFoldable` derive macro)
 - rust-lang#78842 (Honor the rustfmt setting in config.toml)
 - rust-lang#78843 (Less verbose debug logging from inlining integrator)
 - rust-lang#78852 (Convert a bunch of intra-doc links)
 - rust-lang#78860 (rustc_resolve: Use `#![feature(format_args_capture)]`)
 - rust-lang#78861 (typo and formatting)
 - rust-lang#78865 (Don't fire `CONST_ITEM_MUTATION` lint when borrowing a deref)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1c66688 into rust-lang:master Nov 8, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 8, 2020
@tshepang tshepang deleted the nits branch November 8, 2020 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants