From 1e9e8a4a7750c6dfca59e04e7a8add297ddb10be Mon Sep 17 00:00:00 2001 From: Enrico Seiler Date: Mon, 5 Feb 2024 13:10:52 +0100 Subject: [PATCH] [FIX] is_option_set: match both long and short ids --- include/sharg/config.hpp | 12 ++++ include/sharg/detail/format_parse.hpp | 69 +++++++++++-------- include/sharg/parser.hpp | 94 ++++++++++++++++++-------- test/unit/parser/format_parse_test.cpp | 10 ++- 4 files changed, 128 insertions(+), 57 deletions(-) diff --git a/include/sharg/config.hpp b/include/sharg/config.hpp index 0ca8e7e6..970d097d 100644 --- a/include/sharg/config.hpp +++ b/include/sharg/config.hpp @@ -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 * diff --git a/include/sharg/detail/format_parse.hpp b/include/sharg/detail/format_parse.hpp index 3af883f9..305b4e5e 100644 --- a/include/sharg/detail/format_parse.hpp +++ b/include/sharg/detail/format_parse.hpp @@ -145,10 +145,10 @@ class format_parse : public format_base template static bool is_empty_id(id_type const & id) { - if constexpr (std::same_as, std::string>) - return id.empty(); - else // char + if constexpr (std::same_as) return id == '\0'; + else + return id.empty(); } /*!\brief Finds the position of a short/long identifier in format_parse::argv. @@ -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. * @@ -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 - static iterator_type find_option_id(iterator_type begin_it, iterator_type end_it, id_type const & id) + template + 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) // 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 + 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 + 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: diff --git a/include/sharg/parser.hpp b/include/sharg/parser.hpp index 26169797..5810b6ae 100644 --- a/include/sharg/parser.hpp +++ b/include/sharg/parser.hpp @@ -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; + // 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, 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; + char_or_string_t const short_or_long_id{id}; // e.g. convert char * to string here if necessary - if constexpr (!std::same_as) // long id was given + if constexpr (id_is_long) // long id was given { if (short_or_long_id.size() == 1) { @@ -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; } @@ -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 used_option_ids{"h", "hh", "help", "advanced-help", "export-help", "version", "copyright"}; + std::set used_option_ids{{"h", "help"}, + {"hh", "advanced-help"}, + {"", "export-help"}, + {"", "version"}, + {"", "copyright"}}; //!\brief The command line arguments. std::vector cmd_arguments{}; @@ -884,11 +895,34 @@ class parser * otherwise. */ template - bool id_exists(id_type const & id) + std::optional 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) + { + 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. @@ -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. diff --git a/test/unit/parser/format_parse_test.cpp b/test/unit/parser/format_parse_test.cpp index 4fe1a659..fbda23d2 100644 --- a/test/unit/parser/format_parse_test.cpp +++ b/test/unit/parser/format_parse_test.cpp @@ -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"}); @@ -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