Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Parse command line arguments as a std::string #330

Closed
wants to merge 5 commits into from
Closed

Parse command line arguments as a std::string #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95c7709
Parse command line input wrapped in a string
huangzonghao Apr 29, 2022
4f7ab89
Update example code and readme for string parsing
huangzonghao May 1, 2022
fb50260
Use constref for string, smart pointer for char array, and address co…
huangzonghao May 2, 2022
6c6deab
Add test cases for string parsing
huangzonghao May 3, 2022
ca98684
Revert changes made to example.cpp in 4f7ab89
huangzonghao May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,16 @@ To parse the command line do:
auto result = options.parse(argc, argv);
```

Or if you have all the arguments of `argv` in a `std::string`, separated by
Copy link
Contributor

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 a char*? I can't wrap it in a string, so you're forcing me to string-copy.

Copy link
Author

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.

Copy link
Contributor

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.

white spaces, do:

```cpp
auto result = options.parse(argv_str);
```

Note the support for string parsing is limited. No bash variable replacement and
wild card expansion.

To retrieve an option use `result.count("option")` to get the number of times
it appeared, and

Expand Down
26 changes: 26 additions & 0 deletions include/cxxopts.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "");

Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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()]);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 string_view" or "poor man's span" in some of my C++11 projects.

Copy link
Owner

Choose a reason for hiding this comment

The 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 argv vector.

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)
{
Expand Down
Loading