Skip to content

use of string_view in as<T> method #1187

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

Merged
merged 4 commits into from
Aug 9, 2025
Merged

use of string_view in as<T> method #1187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7204a30
Address Issue #881, allowing use of string_view in the as<XX> method …
phlptp Jul 26, 2025
62363b6
style: pre-commit.ci fixes
pre-commit-ci[bot] Jul 26, 2025
46b962d
clean up code
phlptp Jul 26, 2025
6ec893b
style: pre-commit.ci fixes
pre-commit-ci[bot] Jul 26, 2025
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
6 changes: 4 additions & 2 deletions include/CLI/Option.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ class Option : public OptionBase<Option> {
/// complete Results of parsing
results_t results_{};
/// results after reduction
results_t proc_results_{};
mutable results_t proc_results_{};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mutable? Generally that means this is thread safe, while results_t won't necessarily be thread safe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key issue is the results and related as<T> method. These methods are const as normally they should not result in changes to the data structure. This converts the results to whatever is requested. But in cases where the results are requested as a string_view and there was no data given in the parsing, and there was a default value given. This can lead to undefined behavior in the way we were doing it, as the string_view could end up pointing to a temporary. Some compilers handled this gracefully others didn't. The simplest lowest impact solution to this was to make proc_results which is where processed results are stored in the mutable object so it can be modified in a const method in those few select cases and prevent undefined behavior. The other options were to just ban string_view but would not solve the underlying problem, just the most common exemplar of it. Make some more complicated storage buffers, or force processing for all options to pre fill the results structures. My assessment was that this was the least intrusive and simplest option.

/// enumeration for the option state machine
enum class option_state : char {
parsing = 0, //!< The option is currently collecting parsed results
Expand Down Expand Up @@ -700,7 +700,9 @@ class Option : public OptionBase<Option> {
} else {
res = reduced_results();
}
retval = detail::lexical_conversion<T, T>(res, output);
// store the results in a stable location if the output is a view
proc_results_ = std::move(res);
retval = detail::lexical_conversion<T, T>(proc_results_, output);
}
if(!retval) {
throw ConversionError(get_name(), results_);
Expand Down
34 changes: 34 additions & 0 deletions tests/OptionTypeTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,40 @@ TEST_CASE_METHOD(TApp, "stringLikeTests", "[optiontype]") {
CHECK("bca" == m_type.m_value);
}

#if CLI11_HAS_FILESYSTEM
#include <string_view>
// test code from https://github.com/CLIUtils/CLI11/issues/881
// https://github.com/Jean1995
TEST_CASE_METHOD(TApp, "AsStringView", "[app]") {
app.add_option("--input", "input option")->default_val("optA")->check(CLI::IsMember({"optA", "optB", "optC"}));

args = {};
run();
auto inputStr = app["--input"]->as<std::string_view>();
CHECK(inputStr == "optA");

args = {"--input", "optC"};
run();
inputStr = app["--input"]->as<std::string_view>();
CHECK(inputStr == "optC");
}

#endif

TEST_CASE_METHOD(TApp, "AsStringRef", "[app]") {
app.add_option("--input", "input option")->default_val("optA")->check(CLI::IsMember({"optA", "optB", "optC"}));

args = {};
run();
const std::string &inputStr = app["--input"]->as<std::string>();
CHECK(inputStr == "optA");

args = {"--input", "optC"};
run();
const std::string &inputStr2 = app["--input"]->as<std::string>();
CHECK(inputStr2 == "optC");
}

TEST_CASE_METHOD(TApp, "VectorExpectedRange", "[optiontype]") {
std::vector<std::string> strvec;

Expand Down
Loading