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 Mar 13, 2024
1 parent 39f65a4 commit e58a7f4
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 54 deletions.
54 changes: 28 additions & 26 deletions include/sharg/detail/format_parse.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <sharg/concept.hpp>
#include <sharg/detail/format_base.hpp>
#include <sharg/detail/id_pair.hpp>

namespace sharg::detail
{
Expand Down Expand Up @@ -142,10 +143,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::arguments.
Expand All @@ -169,32 +170,33 @@ class format_parse : public format_base
* The `id` is found by comparing every argument in arguments 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, detail::id_pair const & id)
{
if (is_empty_id(id))
bool const short_id_empty{id.short_id.empty()};
bool const long_id_empty{id.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, id.short_id.size()) == id.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, id.long_id.size()) == id.long_id // prefix is the same
&& (current_arg.size() == id.long_id.size() + 2
|| current_arg[id.long_id.size() + 2] == '='); // space or `=`

return short_id_found || long_id_found;
};

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

private:
Expand Down
115 changes: 115 additions & 0 deletions include/sharg/detail/id_pair.hpp
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// SPDX-FileCopyrightText: 2006-2024, Knut Reinert & Freie Universität Berlin
// SPDX-FileCopyrightText: 2016-2024, Knut Reinert & MPI für molekulare Genetik
// SPDX-License-Identifier: BSD-3-Clause

/*!\file
* \author Enrico Seiler <enrico.seiler AT fu-berlin.de>
* \brief Provides sharg::detail::id_pair.
*/

#pragma once

#include <string>
#include <unordered_set>

//!\cond

namespace sharg::detail
{

/*!\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.

id_pair() = default;
id_pair(id_pair const &) = default;
id_pair(id_pair &&) = default;
id_pair & operator=(id_pair const &) = default;
id_pair & operator=(id_pair &&) = default;
~id_pair() = default;

id_pair(std::string id_short, std::string id_long) : short_id{std::move(id_short)}, long_id{std::move(id_long)}
{}

id_pair(char const id_short, std::string id_long) :
short_id{std::string(id_short != '\0', id_short)},
long_id{std::move(id_long)}
{}

id_pair(char const id_short) : short_id{std::string(id_short != '\0', id_short)}
{}

id_pair(std::string id_long) : long_id{std::move(id_long)}
{}

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

static auto find(std::unordered_set<id_pair> const & used_ids, char const short_id);

static auto find(std::unordered_set<id_pair> const & used_ids, std::string const & long_id);

static bool contains(std::unordered_set<id_pair> const & used_ids, char const short_id);

static bool contains(std::unordered_set<id_pair> const & used_ids, std::string const & long_id);
};

} // namespace sharg::detail

namespace std
{

template <>
struct hash<sharg::detail::id_pair>
{
size_t operator()(sharg::detail::id_pair const & value) const noexcept
{
size_t h1 = std::hash<std::string>{}(value.short_id);
size_t h2 = std::hash<std::string>{}(value.long_id);
return h1 ^ (h2 << 1);
}
};

} // namespace std

namespace sharg::detail
{

inline auto id_pair::find(std::unordered_set<id_pair> const & used_ids, char const short_id)
{
std::string const id_str(1, short_id);
auto it = std::ranges::find_if(used_ids,
[&id_str](id_pair const & id)
{
return id.short_id == id_str;
});
return it;
}

inline auto id_pair::find(std::unordered_set<id_pair> const & used_ids, std::string const & long_id)
{
auto it = std::ranges::find_if(used_ids,
[&long_id](id_pair const & id)
{
return id.long_id == long_id;
});
return it;
}

inline bool id_pair::contains(std::unordered_set<id_pair> const & used_ids, char const short_id)
{
return id_pair::find(used_ids, short_id) != used_ids.end();
}

inline bool id_pair::contains(std::unordered_set<id_pair> const & used_ids, std::string const & long_id)
{
return id_pair::find(used_ids, long_id) != used_ids.end();
}

} // namespace sharg::detail

//!\endcond
50 changes: 25 additions & 25 deletions include/sharg/parser.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,11 +495,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 @@ -509,13 +511,18 @@ class parser
}
}

if (!used_ids.contains(std::string{id}))
throw design_error{"You can only ask for option identifiers that you added with add_option() before."};
detail::id_pair const & ids = [this, &short_or_long_id]()
{
auto it = detail::id_pair::find(used_option_ids, short_or_long_id);
if (it == used_option_ids.end())
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 `option_end_identifier` (`--`)
auto option_end = std::find(format_arguments.begin(), format_arguments.end(), option_end_identifier);
auto option_it = detail::format_parse::find_option_id(format_arguments.begin(), option_end, short_or_long_id);
return option_it != option_end;
auto end_of_options = std::find(format_arguments.begin(), format_arguments.end(), option_end_identifier);
auto option_it = detail::format_parse::find_option_id(format_arguments.begin(), end_of_options, ids);
return option_it != end_of_options;
}

//!\name Structuring the Help Page
Expand Down Expand Up @@ -741,8 +748,12 @@ class parser
detail::format_copyright>
format{detail::format_short_help{}};

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

//!\brief The command line arguments that will be passed to the format.
std::vector<std::string> format_arguments{};
Expand Down Expand Up @@ -944,19 +955,6 @@ class parser
format = detail::format_parse(format_arguments);
}

/*!\brief Checks whether the long identifier has already been used before.
* \param[in] id The long identifier of the command line option/flag.
* \returns `true` if an option or flag with the long identifier exists or `false`
* otherwise.
*/
template <typename id_type>
bool id_exists(id_type const & id)
{
if (detail::format_parse::is_empty_id(id))
return false;
return (!(used_ids.insert(std::string({id}))).second);
}

/*!\brief Verifies that the short and the long identifiers are correctly formatted.
* \param[in] short_id The short identifier of the command line option/flag.
* \param[in] long_id The long identifier of the command line option/flag.
Expand All @@ -981,7 +979,7 @@ class parser
{
if (short_id == '-' || !is_valid(short_id))
throw design_error{"Short identifiers may only contain alphanumeric characters, '_', or '@'."};
if (id_exists(short_id))
if (detail::id_pair::contains(used_option_ids, short_id))
throw design_error{"Short identifier '" + std::string(1, short_id) + "' was already used before."};
}

Expand All @@ -993,9 +991,11 @@ class parser
throw design_error{"Long identifiers may not use '-' as first character."};
if (!std::ranges::all_of(long_id, is_valid))
throw design_error{"Long identifiers may only contain alphanumeric characters, '_', '-', or '@'."};
if (id_exists(long_id))
if (detail::id_pair::contains(used_option_ids, long_id))
throw design_error{"Long identifier '" + long_id + "' was already used before."};
}

used_option_ids.emplace(short_id, long_id);
}

//!brief Verify the configuration given to a sharg::parser::add_option call.
Expand Down
15 changes: 12 additions & 3 deletions test/unit/parser/format_parse_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -732,21 +732,30 @@ TEST_F(format_parse_test, issue1544)
TEST_F(format_parse_test, is_option_set)
{
std::string option_value{};
std::vector<std::string> positional_options{};

auto parser = get_parser("-l", "hallo", "--foobar", "ballo", "--");
auto parser = get_parser("-l", "hallo", "--foobar", "ballo", "--", "--loo", "--bar");
// `--` signals the end of options!
parser.add_positional_option(positional_options, sharg::config{});
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"});

// you cannot call option_is_set before parse()
EXPECT_THROW(parser.is_option_set("foo"), sharg::design_error);

EXPECT_NO_THROW(parser.parse());
EXPECT_EQ(positional_options.size(), 2u);
EXPECT_EQ(positional_options[0], "--loo");
EXPECT_EQ(positional_options[1], "--bar");

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"));
EXPECT_FALSE(parser.is_option_set('b'));
EXPECT_FALSE(parser.is_option_set("bar")); // --bar is behind the `--`

// errors:
auto expect_design_error = [&](auto && option)
Expand Down

0 comments on commit e58a7f4

Please sign in to comment.