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.

Sign up for GitHub

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

Jump to bottom

Command line processing of --threads argument prevents users from having any commandline arguments starting with --threads #760

Closed
jjellio opened this issue Apr 27, 2017 · 2 comments
Closed

Command line processing of --threads argument prevents users from having any commandline arguments starting with --threads #760

jjellio opened this issue Apr 27, 2017 · 2 comments
Assignees
@ibaned
Labels
Enhancement Improve existing capability; will potentially require voting

Comments

@jjellio
Copy link
Contributor

jjellio commented Apr 27, 2017

Kokkos takes ownership of all command line arguments starting with '--threads' (--numa, device, ndevice as well). This prevents users from having any command line arguments starting with these strings, e.g., --threads_for_me, --threadsA, etc...

The logic at line 251 only checks that the argument starts with the specified string, not that the string starts with AND ends with '=' (line 253)

kokkos/core/src/impl/Kokkos_Core.cpp

Line 251 in 36b92f4

if ((strncmp(arg[iarg],"--kokkos-threads",16) == 0) || (strncmp(arg[iarg],"--threads",9) == 0)) {

@ibaned ibaned added the Enhancement Improve existing capability; will potentially require voting label Apr 28, 2017
@ibaned
Copy link
Contributor

ibaned commented Apr 28, 2017

It looks like you're suggesting moving the conditional on line 253 to line 251, which seems perfectly reasonable to me, at least for the --threads variant. Unless someone else on the team objects, we can probably do this.

@ibaned ibaned self-assigned this May 16, 2017
ibaned added a commit that referenced this issue May 16, 2017
@ibaned
cleanup command line parsing
e11b9c9 
hoist common code into check_arg()
and check_int_arg().
check_arg() addresses issue #760 by
skipping over options like '--threads-for-me'
instead of throwing an exception that
'--threads' was given wrong.
@ibaned ibaned mentioned this issue May 16, 2017
Merged
@ibaned
Copy link
Contributor

ibaned commented May 16, 2017

I've implemented this more generally in PR #800. If that is accepted this will be fixed in the develop branch.

@crtrott crtrott closed this as completed May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ibaned ibaned

Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ibaned @crtrott @jjellio