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

Use a Result for get_physical() ? #43

Open
sector-f opened this Issue Jun 2, 2017 · 9 comments

Comments

Projects
None yet
7 participants
@sector-f
Copy link

sector-f commented Jun 2, 2017

The current signature is pub fn get_physical() -> usize. The documentation says that it will return the same as get() if it isn't able to get the number of physical cores on that architecture. I'm wondering if it would be a good idea to have get_physical() return a Result<usize, usize> to inform the user of whether that fallback was utilized or not.

@seanmonstar

This comment has been minimized.

Copy link
Owner

seanmonstar commented Jun 2, 2017

I don't know if that is useful for not, just pointing out that that would be a breaking change to the API, so would require going to v2 to make that change.

@sector-f

This comment has been minimized.

Copy link
Author

sector-f commented Jun 2, 2017

I don't know if it would be useful or not either; the thought just crossed my mind and I figured I would share

@GabrielMajeri

This comment has been minimized.

Copy link
Contributor

GabrielMajeri commented Jun 22, 2017

Perhaps there could be a try_get_physical function that returns an Option<usize>, and the get_physical function stays the same, while calling try_get_physical under the hood. That way the current API stays the same.

I don't know if it's useful either - if this library is unable to find the number of physical cores, I doubt the user would have a better fallback.

@budziq

This comment has been minimized.

Copy link

budziq commented Sep 30, 2017

I'd argue that there might be a usecase (although I'm not able to give one from the top of my head) in which user would like to be certain about number of physical cores, and if the query fails, they might make an informed decision instead of automatic fallback.

@bobbo bobbo referenced this issue Oct 18, 2017

Open

num_cpus library evaluation tracking issue #55

5 of 6 tasks complete
@bobbo

This comment has been minimized.

Copy link
Contributor

bobbo commented Oct 18, 2017

There's been quite a bit of discussion on this one (and the best return types for these functions in general) at the Libz Blitz evaluation thread: https://internals.rust-lang.org/t/out-of-band-crate-evaluation-for-2017-10-13-num-cpus/5986/2

Have linked to the evaluation tracking issue.

@KodrAus

This comment has been minimized.

Copy link

KodrAus commented Nov 8, 2017

Does anyone have any more thoughts on this? If we can't find a compelling use-case then I'd be inclined to leave the status-quo unchanged, and avoid the possible confusion of users not knowing whether they should care.

What do you think?

@bobbo

This comment has been minimized.

Copy link
Contributor

bobbo commented Nov 15, 2017

I'm struggling to come up with a compelling use case for this, to be totally honest. It definitely feels like the right thing to do is return a Result or Option here, but if there's nothing the library user can usefully do with the extra information, we might as well not change the API.

@seanmonstar any thoughts to add?

@Restioson

This comment has been minimized.

Copy link

Restioson commented Nov 18, 2017

The user probably can find a way to use this extra info -- for instance, only displaying only logical CPUs. It might also matter for very low level stuff, or hardware discovery. IMO if you can embed more information unobtrusively, then you should. The only downside is having to bump to 2.0... but that's what SemVer's for, right? No use in having major versions if you don't use 'em 😛

@KodrAus

This comment has been minimized.

Copy link

KodrAus commented Feb 13, 2018

Circling back around to this. Are we all happy to just close?

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.