Use cxxopts for command line arguments #86
Conversation
src/cli/main.cpp
Outdated
| return cxxopts::value<double>()->default_value(defs); | ||
| } | ||
|
|
||
| auto int_with_default(const char* defs) -> decltype(cxxopts::value<int>()->default_value("")) |
There was a problem hiding this comment.
@iglesias any idea how to avoid code duplication here? It seems template aliases are only supported for types but I wanted to avoid introducing classes.
There was a problem hiding this comment.
Yeah, it's in #87. I was doubting if the return type deduction would be problematic with compiler version but locally with gcc 13.2 it worked just fine without tweaking any options. I don't know in the CI yet. Can you take a look and try if it works well on your end too?
There was a problem hiding this comment.
Would it make sense trying to simplify more passing int, double or std::string (instead of the literals)? Deducing the type and then no need to pass it, e.g. toward with_default(2) instead of int_with_default("2") or with_default<int>("2").
src/cli/main.cpp
Outdated
| { | ||
| processed.push_back(argv[i]); | ||
| #if defined(USE_SLASH_CLI_WINDOWS) && (defined(_WIN32) || defined(_WIN64)) | ||
| // rpplace -- and - with / |
There was a problem hiding this comment.
It is a complicated story here.
We can basically replace / with -- and pass it to the parsing library. But this doesn't help with shortened keywords because they have to be replaced with -. The library doesn't support anything about that with '-' being hardcoded (sic!).
I am in favour of just dropping the slash support here.
@iglesias curious what you think.
There was a problem hiding this comment.
Other options might be to drop support of short keywords. The verbose style of WIndows commands would match that, too. We can also replace - style with CamelCase.
There was a problem hiding this comment.
Yeah... I think I see.
Among those two options, I would definitely choose dropping the slash support. It's subjective, though. When going through cxxopts README, I got the idea that one its main features is supporting long and short keywords.
Something involving some more work, but I think would help to replace / with -- or -:
At the moment the long options are defined as static const char*s and then, each associated with the short version via calls to either.
Instead of the const char*s, I think we could use some kind of map from empty structs to string, or a set of strings if the tags aren't essential. At the moment of replacing, if it is in the map's values or in the set, then replace with --, detected as a long option, otherwise with -. Would that make sense, or would it clash with the error handling for unknown options? If the latter, then the mapping should associate to short options too and make two lookups.
In case it sounds good, I could work on this during this week. Or I could also let you to it and review, that's also fine of course. Any option is good :)
There was a problem hiding this comment.
Dropping it is then! Let me just commit it right away.
I think it is way too complicated to be worth spending your time.
lisitsyn
force-pushed
the
refactor/cxxopts
branch
from
b166065 to
4a9d442
Compare
|
@iglesias looks like it is ready to go |
lisitsyn
changed the title
cxxoptsseems to be a go-to solution for command line options parsing nowadays. Using something well-supported seems better than freezing parser in the code. Also, this cleans up the licensing part.