-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[main] Introduce cmdline option parser to share some code between executables #20073
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 16 files 16 suites 2d 18h 50m 30s ⏱️ Results for commit b91ae6a. |
/// // NOTE: `args` should not contain the program name! It should usually be `argc + 1`. | ||
/// opts.Parse(args, nArgs); |
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.
/// // NOTE: `args` should not contain the program name! It should usually be `argc + 1`. | |
/// opts.Parse(args, nArgs); | |
/// // NOTE: `args` should not contain the program name! It should usually be `argc + 1`. | |
/// // For example `main(char **argv, int argc)` might call this function as: | |
/// // `ParseArgs(const_cast<const_cast<const char**>(argv) + 1, argc - 1);` | |
/// opts.Parse(args, nArgs); |
(Without this, I was side tracked for a while pondering whether the comment wanted the last line to be opts.Parse(args, nArgs+1)
)
ROOT::RCmdLineOpts opts; | ||
opts.AddFlag({"--foo"}, ROOT::RCmdLineOpts::EFlagType::kWithArg); | ||
|
||
const char *args[] = {"--foo", "bar"}; |
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.
Do we test something like:
const char *args[] = {"somename", "--foo", "bar", "-abc"};
and
const char *args[] = {"somename", "--noargopt", "bar"};
const char *args[] = {"--noargopt", "bar"};
i.e. location of positional arguments. (In particular the first one might appear by mistake if argv
is passed instead of argv+1
)
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.
LGTM (Beside the comments and portability issue)
Thanks a lot for the initiative! Even if it's 300 lines of code, isn't it better to rely on something external that gets auto-maintained, even if it's bigger in number of lines of code? Or to understand better the motivation, what do we gain by having a small parser for which we have to add our own tests?
|
It's mostly the last point (though the first is also somewhat important). |
We now have enough C++ executables (with more to be ported) to justify sharing some code between them, in particular option parsing.
This PR introduces a simple option parser class that covers most of our cases. Not all cases (e.g.
hadd
keeps its custom parsing because it's a bit weird), but it's already enough to coverrootls
androotbrowse
, and will in the future also coverrootcp
,rootmv
etc.The parser is documented with code examples and properly tested and it's about 300 lines of codes (about an order of magnitude less than e.g. cxxopts which in my opinion is overkill for our use case).
Checklist: