Allow passing arguments as options after the -- option#616
Allow passing arguments as options after the -- option#616Axel-Naumann merged 1 commit intomasterfrom unknown repository
Conversation
|
Can one of the admins verify this patch? |
Axel-Naumann
left a comment
Axel-Naumann
left a comment
There was a problem hiding this comment.
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.
Spelling: two times a macro is two "macros" :-)
There was a problem hiding this comment.
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.
Don't we have a TString function which can replace the strchr? Like Index() or something?
core/base/src/TApplication.cxx
Outdated
| Warning("GetOptions", "-e must be followed by an expression."); | ||
| } | ||
| } else if (!strcmp(argv[i], "--")) { | ||
| TObjString* macro = NULL; |
There was a problem hiding this comment.
Can you use nullptr instead of NULL? (I know, not obvious when looking at surrounding code...)
| } | ||
|
|
||
| if (macro) { | ||
| argv[i] = null; |
There was a problem hiding this comment.
Should this null be nullptr?
There was a problem hiding this comment.
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.
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.
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".
ghost
mentioned this pull request
Axel-Naumann
left a comment
Axel-Naumann
left a comment
There was a problem hiding this comment.
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.
Ha! Funny syntax :-) Nonetheless - could you reword this to the more common (for ROOT)
for (auto f: *fFiles)
| } | ||
|
|
||
| if (macro) { | ||
| argv[i] = null; |
There was a problem hiding this comment.
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! |
3 participants
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);.rootfiles;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.