Skip to content

Commit

Permalink
[FIX] is_option_set: match both long and short ids
Browse files Browse the repository at this point in the history
  • Loading branch information
@eseiler
eseiler committed Feb 7, 2024
1 parent fc521fe commit 1e9e8a4
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 57 deletions.
12 changes: 12 additions & 0 deletions include/sharg/config.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,18 @@
namespace sharg
{

/*!\brief A simple struct to store a short and a long identifier for an option.
* \ingroup parser
*/
struct id_pair
{
std::string short_id; //!< The short identifier for the option.
std::string long_id; //!< The long identifier for the option.

//!\brief Compares two id_pairs for equality.
friend auto operator<=>(id_pair const &, id_pair const &) = default;
};

/*!\brief Option struct that is passed to the `sharg::parser::add_option()` function.
* \ingroup parser
*
Expand Down
69 changes: 42 additions & 27 deletions include/sharg/detail/format_parse.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ class format_parse : public format_base
template <typename id_type>
static bool is_empty_id(id_type const & id)
{
if constexpr (std::same_as<std::remove_cvref_t<id_type>, std::string>)
return id.empty();
else // char
if constexpr (std::same_as<id_type, char>)
return id == '\0';
else
return id.empty();
}

/*!\brief Finds the position of a short/long identifier in format_parse::argv.
Expand All @@ -157,7 +157,7 @@ class format_parse : public format_base
* std::string if it denotes a long identifier.
* \param[in] begin_it The iterator where to start the search of the identifier.
* \param[in] end_it The iterator one past the end of where to search the identifier.
* \param[in] id The identifier to search for (must not contain dashes).
* \param[in] ids The identifier to search for (must not contain dashes).
* \returns An iterator pointing to the first occurrence of `id` in the list pointed to by `begin_it`
* or `end_it` if it is not contained.
*
Expand All @@ -172,32 +172,47 @@ class format_parse : public format_base
* The `id` is found by comparing every argument in argv to `id` prepended with two dashes (`--`)
* or a prefix of such followed by the equal sign `=`.
*/
template <typename iterator_type, typename id_type>
static iterator_type find_option_id(iterator_type begin_it, iterator_type end_it, id_type const & id)
template <typename iterator_type>
static iterator_type find_option_id(iterator_type begin_it, iterator_type end_it, id_pair const & ids)
{
if (is_empty_id(id))
bool short_id_empty{ids.short_id.empty()};
bool long_id_empty{ids.long_id.empty()};

if (short_id_empty && long_id_empty)
return end_it;

return (std::find_if(begin_it,
end_it,
[&](std::string const & current_arg)
{
std::string full_id = prepend_dash(id);

if constexpr (std::same_as<id_type, char>) // short id
{
// check if current_arg starts with "-o", i.e. it correctly identifies all short notations:
// "-ovalue", "-o=value", and "-o value".
return current_arg.substr(0, full_id.size()) == full_id;
}
else
{
// only "--opt Value" or "--opt=Value" are valid
return current_arg.substr(0, full_id.size()) == full_id && // prefix is the same
(current_arg.size() == full_id.size()
|| current_arg[full_id.size()] == '='); // space or `=`
}
}));
auto cmp = [&](std::string const & current_arg)
{
// check if current_arg starts with "-o", i.e. it correctly identifies all short notations:
// "-ovalue", "-o=value", and "-o value".
bool const short_id_found = !short_id_empty && current_arg.starts_with("-")
&& current_arg.substr(1, ids.short_id.size()) == ids.short_id;

// only "--opt Value" or "--opt=Value" are valid
bool const long_id_found = !short_id_found // Don't need to check long_id if short_id was found
&& !long_id_empty && current_arg.starts_with("--")
&& current_arg.substr(2, ids.long_id.size()) == ids.long_id // prefix is the same
&& (current_arg.size() == ids.long_id.size() + 2
|| current_arg[ids.long_id.size() + 2] == '='); // space or `=`

return short_id_found || long_id_found;
};

return std::find_if(begin_it, end_it, cmp);
}

//!\overload
template <typename iterator_type>
static iterator_type find_option_id(iterator_type begin_it, iterator_type end_it, char const id)
{
return find_option_id(std::move(begin_it), std::move(end_it), id_pair{std::string(1, id), ""});
}

//!\overload
template <typename iterator_type>
static iterator_type find_option_id(iterator_type begin_it, iterator_type end_it, std::string const & id)
{
return find_option_id(std::move(begin_it), std::move(end_it), id_pair{"", id});
}

private:
Expand Down
94 changes: 67 additions & 27 deletions include/sharg/parser.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,11 +500,13 @@ class parser
if (!parse_was_called)
throw design_error{"You can only ask which options have been set after calling the function `parse()`."};

constexpr bool id_is_long = !std::same_as<id_type, char>;

// the detail::format_parse::find_option_id call in the end expects either a char or std::string
using char_or_string_t = std::conditional_t<std::same_as<id_type, char>, char, std::string>;
char_or_string_t short_or_long_id = {id}; // e.g. convert char * to string here if necessary
using char_or_string_t = std::conditional_t<id_is_long, std::string, char>;
char_or_string_t const short_or_long_id{id}; // e.g. convert char * to string here if necessary

if constexpr (!std::same_as<id_type, char>) // long id was given
if constexpr (id_is_long) // long id was given
{
if (short_or_long_id.size() == 1)
{
Expand All @@ -514,12 +516,17 @@ class parser
}
}

if (std::find(used_option_ids.begin(), used_option_ids.end(), std::string{id}) == used_option_ids.end())
throw design_error{"You can only ask for option identifiers that you added with add_option() before."};
id_pair const & ids = [this, &short_or_long_id]()
{
auto it = find_used_id(short_or_long_id);
if (!it)
throw design_error{"You can only ask for option identifiers that you added with add_option() before."};
return *it;
}();

// we only need to search for an option before the `end_of_options_indentifier` (`--`)
auto end_of_options = std::find(cmd_arguments.begin(), cmd_arguments.end(), end_of_options_indentifier);
auto option_it = detail::format_parse::find_option_id(cmd_arguments.begin(), end_of_options, short_or_long_id);
auto option_it = detail::format_parse::find_option_id(cmd_arguments.begin(), end_of_options, ids);
return option_it != end_of_options;
}

Expand Down Expand Up @@ -715,7 +722,11 @@ class parser
format{detail::format_help{{}, {}, false}}; // Will be overwritten in any case.

//!\brief List of option/flag identifiers that are already used.
std::set<std::string> used_option_ids{"h", "hh", "help", "advanced-help", "export-help", "version", "copyright"};
std::set<id_pair> used_option_ids{{"h", "help"},
{"hh", "advanced-help"},
{"", "export-help"},
{"", "version"},
{"", "copyright"}};

//!\brief The command line arguments.
std::vector<std::string> cmd_arguments{};
Expand Down Expand Up @@ -884,11 +895,34 @@ class parser
* otherwise.
*/
template <typename id_type>
bool id_exists(id_type const & id)
std::optional<id_pair> find_used_id(id_type const & id) const
{
if (detail::format_parse::is_empty_id(id))
return false;
return (!(used_option_ids.insert(std::string({id}))).second);
return std::nullopt;

if constexpr (std::same_as<id_type, char>)
{
std::string const id_str{std::string(1, id)};
auto it = std::ranges::find_if(used_option_ids,
[&id_str](id_pair const & element)
{
return element.short_id == id_str;
});
if (it != used_option_ids.end())
return *it;
}
else
{
auto it = std::ranges::find_if(used_option_ids,
[&id](id_pair const & element)
{
return element.long_id == id;
});
if (it != used_option_ids.end())
return *it;
}

return std::nullopt;
}

/*!\brief Verifies that the short and the long identifiers are correctly formatted.
Expand All @@ -908,27 +942,33 @@ class parser
return valid_chars.find(c) != std::string::npos;
};

if (id_exists(short_id))
throw design_error("Option Identifier '" + std::string(1, short_id) + "' was already used before.");
if (id_exists(long_id))
throw design_error("Option Identifier '" + long_id + "' was already used before.");
bool const short_id_empty = detail::format_parse::is_empty_id(short_id);
bool const long_id_empty = detail::format_parse::is_empty_id(long_id);
bool const long_id_valid = std::ranges::all_of(long_id,
[&is_valid](char c)
{
return ((c == '-') || is_valid(c));
});

// Validity
if (short_id_empty && long_id_empty)
throw design_error("Option Identifiers cannot both be empty.");
if (!short_id_empty && !is_valid(short_id))
throw design_error("Option identifiers may only contain alphanumeric characters, '_', or '@'.");
if (long_id.length() == 1)
throw design_error("Long IDs must be either empty, or longer than one character.");
if ((short_id != '\0') && !is_valid(short_id))
throw design_error("Option identifiers may only contain alphanumeric characters, '_', or '@'.");
if (long_id.size() > 0 && (long_id[0] == '-'))
if (!long_id_empty && (long_id[0] == '-'))
throw design_error("First character of long ID cannot be '-'.");
if (!long_id_empty && !long_id_valid)
throw design_error("Long identifiers may only contain alphanumeric characters, '_', '-', or '@'.");

std::for_each(long_id.begin(),
long_id.end(),
[&is_valid](char c)
{
if (!((c == '-') || is_valid(c)))
throw design_error(
"Long identifiers may only contain alphanumeric characters, '_', '-', or '@'.");
});
if (detail::format_parse::is_empty_id(short_id) && detail::format_parse::is_empty_id(long_id))
throw design_error("Option Identifiers cannot both be empty.");
// Availability
if (find_used_id(short_id))
throw design_error("Option Identifier '" + std::string(1, short_id) + "' was already used before.");
if (find_used_id(long_id))
throw design_error("Option Identifier '" + long_id + "' was already used before.");

used_option_ids.insert({std::string(1, short_id), long_id});
}

//!brief Verify the configuration given to a sharg::parser::add_option call.
Expand Down
10 changes: 7 additions & 3 deletions test/unit/parser/format_parse_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -960,8 +960,10 @@ TEST(parse_test, issue1544)
TEST(parse_test, is_option_set)
{
std::string option_value{};
char const * argv[] = {"./parser_test", "-l", "hallo", "--foobar", "ballo", "--", "--loo"};
// `--` signals the end of options!
char const * argv[] = {"./parser_test", "-l", "hallo", "--foobar", "ballo", "--", "--loo", "--bar"};
sharg::parser parser{"test_parser", 5, argv, sharg::update_notifications::off};
parser.add_option(option_value, sharg::config{.short_id = 'b', .long_id = "bar"});
parser.add_option(option_value, sharg::config{.short_id = 'l', .long_id = "loo"});
parser.add_option(option_value, sharg::config{.short_id = 'f', .long_id = "foobar"});

Expand All @@ -970,10 +972,12 @@ TEST(parse_test, is_option_set)
EXPECT_NO_THROW(parser.parse());

EXPECT_TRUE(parser.is_option_set('l'));
EXPECT_TRUE(parser.is_option_set("loo")); // --loo is behind the `--`, but -l is before
EXPECT_TRUE(parser.is_option_set('f'));
EXPECT_TRUE(parser.is_option_set("foobar"));

EXPECT_FALSE(parser.is_option_set('f'));
EXPECT_FALSE(parser.is_option_set("loo")); // --loo is behind the `--` which signals the end of options!
EXPECT_FALSE(parser.is_option_set('b'));
EXPECT_FALSE(parser.is_option_set("bar")); // --bar is behind the `--`

// errors:
EXPECT_THROW(parser.is_option_set("l"), sharg::design_error); // short identifiers are passed as chars not strings
Expand Down

0 comments on commit 1e9e8a4

Please sign in to comment.