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

Add more documentation #61

Merged
merged 3 commits into from Oct 18, 2017

Conversation

Projects
None yet
3 participants
@derekdreery
Copy link
Contributor

derekdreery commented Oct 17, 2017

Hope it is useful. Please read before you accept to make sure you're happy.

@derekdreery derekdreery referenced this pull request Oct 17, 2017

Open

num_cpus library evaluation tracking issue #55

5 of 6 tasks complete
src/lib.rs Outdated
/// thread does not have access to all the computer's cpus.
///
/// [smt]: https://en.wikipedia.org/wiki/Simultaneous_multithreading
/// [sched affinity]: https://en.wikipedia.org/wiki/Simultaneous_multithreading

This comment has been minimized.

@seanmonstar

seanmonstar Oct 17, 2017

Owner

Looks like these 2 links are the same :D

src/lib.rs Outdated
///
/// ```
/// let logical_cpus = num_cpus::get();
/// let physical_cpus = num_cpus::get();

This comment has been minimized.

@seanmonstar

seanmonstar Oct 17, 2017

Owner

I suspect this should be num_cpus::get_physical()?

This comment has been minimized.

@derekdreery

derekdreery Oct 18, 2017

Author Contributor

yup :)

@GabrielMajeri
Copy link
Contributor

GabrielMajeri left a comment

There are some minor issues that need to get fixed.

src/lib.rs Outdated
//! A crate with utilities to determine the number of CPUs available on the
//! current system.
//!
//! Sometimes the cpu will exaggerate the number of cpus it contains, because it can use

This comment has been minimized.

@GabrielMajeri

GabrielMajeri Oct 18, 2017

Contributor

I think it would be a good idea to stick to writing "CPU" or "CPUs" in capital letters, for consistency.

src/lib.rs Outdated
//! number 8, it could use the number of cpus).
//!
//! [processor tricks]: https://en.wikipedia.org/wiki/Simultaneous_multithreading
//! [`rayon::ThreadPool`]: https://doc.rs/rayon/0.8.2/rayon/struct.ThreadPool.html

This comment has been minimized.

@GabrielMajeri

GabrielMajeri Oct 18, 2017

Contributor

Link is broken, should be https://docs.rs/rayon/0.8.2/rayon/struct.ThreadPool.html (instead of doc.rs)

src/lib.rs Outdated
@@ -18,10 +36,29 @@ extern crate libc;


/// Returns the number of available CPUs of the current system.
///
/// This function will get the number of logical cores. Sometimes this is different to the number

This comment has been minimized.

@GabrielMajeri

GabrielMajeri Oct 18, 2017

Contributor

Typo: should be different from the....

Maybe we should rewrite it as "This number might be different from the number of..." instead of "Sometimes this is different..."

This comment has been minimized.

@derekdreery

derekdreery Oct 18, 2017

Author Contributor

I think they're both valid English, but I'll change to from.

I've looked this up and apparently "different to" is British English mainly, so "different from" is probably more international: https://en.oxforddictionaries.com/usage/different-from-than-or-to

src/lib.rs Outdated
@@ -18,10 +36,29 @@ extern crate libc;


/// Returns the number of available CPUs of the current system.
///
/// This function will get the number of logical cores. Sometimes this is different to the number
/// of physical cores becuase the cpu chip can deliver better performance when using more threads.

This comment has been minimized.

@GabrielMajeri

GabrielMajeri Oct 18, 2017

Contributor

Whether SMT actually delivers more performance or not is debatable.

I think we should just stick to saying that some systems have multiple logical cores per physical CPU core, and link to "Simultaneous multithreading" as an example.

This comment has been minimized.

@derekdreery

derekdreery Oct 18, 2017

Author Contributor

Agreed.

src/lib.rs Outdated
///
/// This function will get the number of logical cores. Sometimes this is different to the number
/// of physical cores becuase the cpu chip can deliver better performance when using more threads.
/// (See [simultaneous multithreading on wikipedia][smt]).

This comment has been minimized.

@GabrielMajeri

GabrielMajeri Oct 18, 2017

Contributor

"Simultaneous" and "Wikipedia" should begin with capitals.

@seanmonstar seanmonstar merged commit 4193d17 into seanmonstar:master Oct 18, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@seanmonstar

This comment has been minimized.

Copy link
Owner

seanmonstar commented Oct 18, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.