-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow passing arguments as options after the -- option #616
Conversation
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for coding this! Do my (tiny) comments make sense?
core/base/src/TApplication.cxx
Outdated
continue; | ||
|
||
if (macro) | ||
Warning("GetOptions", "-- is used with several macroses. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling: two times a macro is two "macros" :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything that's not a merge commit is perfectly fine: amend, rebase, add commits - whatever works for you!
core/base/src/TApplication.cxx
Outdated
continue; | ||
if (file->String().EndsWith(".root")) | ||
continue; | ||
if (strchr(file->String().Data(), '(')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have a TString function which can replace the strchr
? Like Index()
or something?
core/base/src/TApplication.cxx
Outdated
@@ -468,7 +468,46 @@ void TApplication::GetOptions(Int_t *argc, char **argv) | |||
} else { | |||
Warning("GetOptions", "-e must be followed by an expression."); | |||
} | |||
} else if (!strcmp(argv[i], "--")) { | |||
TObjString* macro = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use nullptr
instead of NULL
? (I know, not obvious when looking at surrounding code...)
core/base/src/TApplication.cxx
Outdated
} | ||
|
||
if (macro) { | ||
argv[i] = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this null
be nullptr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it shoud not, because later options participate in strcmp
([1]), which doesn't like null pointers. null
is defined as ""
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, indeed, you're right!
@phsft-bot build! |
Starting build on |
core/base/src/TApplication.cxx
Outdated
str += ','; | ||
argv[i] = null; | ||
} | ||
str.EndsWith(",") ? str[str.Length()-1] = ')' : str += ')'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for adding spaces around the "-" in Length() - 1
, I don't think clang-format's suggestions are of much value. So please ignore its "failure".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix this tiny remaining issue?
core/base/src/TApplication.cxx
Outdated
TObjString* macro = nullptr; | ||
|
||
if (fFiles) { | ||
for (auto f = fFiles->GetEntriesFast(); f --> 0; ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! Funny syntax :-) Nonetheless - could you reword this to the more common (for ROOT)
for (auto f: *fFiles)
core/base/src/TApplication.cxx
Outdated
} | ||
|
||
if (macro) { | ||
argv[i] = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, indeed, you're right!
Could you (in a separate PR) hand in a test for this wonderful feature to the roottest repo? Could you also add a few lines about this to README/ReleaseNotes/v612/index.md? |
We still also need some documentation for it (similar to the PR description at least). |
@phsft-bot build! |
Starting build on |
We seem to have two successful builds versus three infrastructure failures - so I suppose that's "green". Merging - thank you for your contribution! |
root macro.C -- arg1 arg2 arg3 ...
will behave asroot macro.C(arg1,arg2,arg3,...)
.Options won't be attached to:
root -e expression
;(
in them, to be precise) —root macro.C(arg1, arg2)
;.root
files;If there are several macros without options, the arguments will be passed to the last one (with warning).
If there are no macros, the options after the
--
will be ignored (with warning).No options description was updated yet.
Initially my idea was to allow to turn macro function parameters into named options, but @Axel-Naumann asked to leave them positional. But I'm still planning to introduce named arguments/options. Positional arguments as options is not a great improvement over the current way to pass arguments to macros.