-
Notifications
You must be signed in to change notification settings - Fork 591
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
Parse command line arguments as a std::string #330
Changes from all commits
95c7709
4f7ab89
fb50260
6c6deab
ca98684
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1772,6 +1772,9 @@ namespace cxxopts | |
ParseResult | ||
parse(int argc, const char* const* argv); | ||
|
||
ParseResult | ||
parse(const std::string& argv_str); | ||
|
||
OptionAdder | ||
add_options(std::string group = ""); | ||
|
||
|
@@ -2284,6 +2287,29 @@ Options::parse(int argc, const char* const* argv) | |
return parser.parse(argc, argv); | ||
} | ||
|
||
inline | ||
ParseResult | ||
Options::parse(const std::string& argv_str) | ||
{ | ||
std::vector<std::string> tokens; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why create all of these copies? |
||
std::string token; | ||
std::istringstream iss(argv_str); | ||
while (std::getline(iss, token, ' ')) | ||
{ | ||
tokens.push_back(token); | ||
} | ||
|
||
std::unique_ptr<const char*[]> argv(new const char*[tokens.size()]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, you're going through a bunch of hoops to create strings, which are copies of the original argv string, which itself may be a copy of something else - and now you're creating copies again? Look, none of this copying needs to happen. Use either a string_view, or your own struct with a pointer and a length, or a pair of pointers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. string_view breaks the requirement of this project, and could potentially break the other projects that depend on this one. But you are right, we should be able to generate argv pointers based on the original string copy. I'll look into this later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @huangzonghao : You're right, of course. By "or" I meant "or, actually, you could use"... Also, one could use string_view lite, which is C++11-compatible. That's essentially the same thing but with more code... I've been known to implement "poor man's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer not to bring in other libraries, and at the moment I still want to keep it C++11 compatible. However I think there is probably a nicer way to do this. I think you could probably just copy the whole string into a new buffer, replace all spaces with a zero, and store the pointers as you go in an |
||
for (std::size_t i = 0; i < tokens.size(); ++i) { | ||
argv[i] = tokens[i].c_str(); | ||
} | ||
|
||
OptionParser parser(*m_options, m_positional, m_allow_unrecognised); | ||
|
||
auto parse_result = parser.parse(static_cast<int>(tokens.size()), argv.get()); | ||
return parse_result; | ||
} | ||
|
||
inline ParseResult | ||
OptionParser::parse(int argc, const char* const* argv) | ||
{ | ||
|
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.
I'm not sure I'm happy with you forcing the use of
std::string
. What if I have achar*
? I can't wrap it in a string, so you're forcing me to string-copy.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.
I think this is a legit concern, though it goes beyond the scope of the problem this patch is trying to target at. Also unlike the single string case, I cannot think of a use case where you are given all the arguments in a single
char*
. I don't see a trivial fix for this to avoid the extra copy of the whole string, but I am happy to learn more on it and address it when possible.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.
@huangzonghao : If you use a class/struct which can be implicitly constructed both from an std::string and from a char* - then you won't need the copy. That class is string_view... see my comment below.