1
\$\begingroup\$

So here's a simple command line parser. Is it safe? I can't use anything but C++14 and standard library.

class cli {
public:
  struct parameter {
    struct help {};
    struct single {};

    template <typename T = void>
    struct value {
      using type = T;
    };

    parameter(const std::string& key)
      : key_{key}
      , description_{""}
      , type_info_(typeid(void)) {
    }

    parameter(const std::string& key, help, const std::string& description = "")
      : key_{key}
      , description_{description}
      , type_info_(typeid(help)) {
    }

    parameter(const std::string& key, single, const std::string& description = "")
      : key_{key}
      , description_{description}
      , type_info_(typeid(single)) {
    }

    template <typename T>
    parameter(const std::string& key, value<T>, const std::string& description, T argument)
      : key_{key}
      , description_{description}
      , type_info_(typeid(T)) {
      std::stringstream ss;
      ss << argument;
      argument_ = ss.str();
    }

    static std::string trim_dashs(const std::string& arg) {
      if (arg.size() > 2 && arg[0] == '-' && arg[1] == '-')
        return {arg.begin() + 2, arg.end()};
      if (arg.size() > 1 && arg[0] == '-')
        return {arg.begin() + 1, arg.end()};
      return arg;
    }

    static bool is_valid_key(const std::string& in) {
      return (in.size() == 2 && in[0] == '-' && in[2] >= 'A' && in[2] <= 'z') ||
             (in.size() >= 3 && in[0] == '-' && in[1] == '-' && in[2] >= 'A' && in[2] <= 'z');
    }

    std::string key() const {
      return key_;
    }

    std::string description() const {
      return description_;
    }

    template <typename T>
    T as() const {
      assert((typeid(T).hash_code() == type_info_.get().hash_code()) ||
             ((typeid(T).hash_code() == typeid(bool).hash_code()) &&
              (typeid(parameter::single).hash_code() == type_info_.get().hash_code())));
      T arg;
      std::stringstream ss(argument_);
      ss >> arg;
      return arg;
    }

    template <typename T = void>
    bool is() const {
      return typeid(T).hash_code() == type_info_.get().hash_code() ||
             typeid(T).hash_code() == typeid(parameter::value<void>).hash_code();
    }

    void argument(const std::string& arg) {
      assert(typeid(parameter::help).hash_code() != type_info_.get().hash_code() &&
             typeid(parameter::single).hash_code() != type_info_.get().hash_code());
      argument_ = arg;
    }

    void argument(bool value) {
      argument_ = value ? "true" : "false";
    }

    bool operator <(const parameter& r) const {
      return key() < r.key();
    }

  private:
    std::string key_;
    std::string description_;
    std::string argument_;
    std::reference_wrapper<const std::type_info> type_info_;
  };

public:
  cli(const std::set<parameter> parameters, int argc, char** argv) 
    : command_line_{parameters} {
    std::vector<std::string> argvector;
    for (int i = 1; i < argc; ++i) {
      argvector.push_back(argv[i]);
    }
    auto is_defined = [this](const std::string& in) {
      return command_line_.find({in, parameter::single()}) != command_line_.end();
    };
    for (auto it = argvector.begin(); it != argvector.end(); ++it) {
      auto key = parameter::trim_dashs(*it);
      if (!parameter::is_valid_key(*it) || !is_defined(key)) {
        throw std::runtime_error(std::string("Unknown parameter '") + key + std::string("'\n") + help_message());
      }

      parameter p = *command_line_.find({key});

      if (p.is<parameter::help>()) {
        std::cerr << help_message() << std::endl;
        exit(0);
      }

      auto next_it = std::next(it);
      const bool is_valid_argument = next_it != argvector.end() && !parameter::is_valid_key(*next_it);
      if (p.is<parameter::value<>>()) {
        if (is_valid_argument) {
          p.argument(*next_it);
          std::advance(it, 1);
        } else {
          throw std::runtime_error(std::string("Missing argument for '") + key + std::string("'\n"));
        }
      } else {
        p.argument("true");
      }

      std::pair<decltype(parameters_)::iterator, bool> inserted = parameters_.insert(p);
      if (not inserted.second)
        throw std::runtime_error(std::string("Double parameter: '") + key + std::string("'\n"));
    }
  }

  const parameter& operator[] (const std::string& key) {
    if (parameters_.count({key}))
      return *parameters_.find({key});
    auto p = command_line_.find({key});
    return *p;
  }

  std::string help_message() const {
    std::string message = "Usage: builder [options]\n"
                          "Allowed options:\n";
    for (auto& argument : command_line_) {
      message += std::string("\t--");
      message += argument.key();
      message += std::string(std::max(0, 10 - static_cast<int>(argument.key().size())), ' ');
      message += std::string(":");
      message += argument.description();
      message += std::string("\n");
    }

    return message;
  }

private:
  std::set<parameter> command_line_;
  std::set<parameter> parameters_;
};

And in the target code:

const std::set<cli::parameter> parameters = {
    {"help", cli::parameter::help(), "help message"},
    {"config", cli::parameter::value<std::string>(), "default configuration", std::string("Debug")},
    {"install", cli::parameter::single(), "build install target"},
    {"pack", cli::parameter::single(), "build package target"},
    {"timeout", cli::parameter::value<uint16_t>(), "timeout in seconds", uint16_t(10)}
  };

cli c(parameters, argc, argv);
foo bar(cli["timeout"].as<uint16_t>());

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Very cool. This is like a bespoke, simplified version of Boost.ProgramOptions. It could definitely be useful.

Unfortunately, the restriction to C++14 (and disallowing any 3rd party libraries) would make it completely useless to me, and most people nowadays, but maybe some people out there are still stuck using C++14.

Let’s dive in:

struct parameter

parameter has an invariant, so by convention, it should be a class, not a struct.

struct help {};
struct single {};

template <typename T = void>
struct value {
  using type = T;
};

I see what you’re trying to do here, but the way you’re doing it is a bit problematic.

To make this kind of design really work, ideally you’d want std::any. Except that’s C++17… and you can’t even use Boost.Any. So to really do this properly, you’re basically going to have to re-implement std::any. That sucks, but… well, you’re using C++14, so you’ll have to deal with it. At least it won’t be (entirely) wasted effort, because it will useful to you anyway (at least until you eventually move to C++17). So you might as well make it.

Here’s why you need std::any (or rather, your own custom version of std::any): Your current design has as a flaw when dealing with typed arguments. If I do builder --timeout xyz, then parse the command line with auto c = cli{parameters, argc, argv}, it will pass without complaint. An error will be raised if I do c["timeout"].as<std::uint16_t>()… but that’s MUCH too late! By that point, I assume the arguments have all been properly parsed, so an error happening here would be jarring. Even worse, the error raised is actually an assertion failure. So rather than getting an error raised at the time the command line is parsed, instead the whole program just… dies… at some random time later.

The reason why std::any fixes this problem is that you can hold an any in the parameter type—rather than a string—and do the checking early, like this:

class parameter
{

    // ... [snip] ...

    template <typename T>
    parameter(std::string key, value<T>, std::string description, T argument)
        : key_{std::move(key)}
        , description_{std::move(description)}
        , argument_{std::move(argument)}
        , type_index_{typeid(T)}
    {}

    // ... [snip] ...

    template <typename T>
    auto as() const -> T
    {
        // will throw a bad_any_cast if the cast is bad
        return std::any_cast<T>(argument_);
    }

    // ... [snip] ...

    template <typename T>
    auto argument(std::string const& arg) -> void
    {
        if (std::type_index(typeid(T)) != type_index_)
        {
            // you really should make a custom exception for this kind of thing
            //
            // then you won't need this long-ass, ugly concatenation
            throw std::runtime_error{"wrong type assigned to option '" 
                + key_
                + "': '"
                + std::string{std::type_index(typeid(T)).name()}
                + "' (expected: '"
                + std::string{type_index_.name()}
                + "')"};
        }

        // it would be nice to avoid the whole stringstream dance if we know
        // the type is easily converted to a string (or is actually a string)
        //
        // this would be trivial in C++17 with if constexpr, but... well,
        // you can figure out how to do it in C++14 on your own time

        auto iss = std::istringstream{arg};

        T value{};
        if (iss >> value)
        {
            argument_ = std::move(value);
        }
        else
        {
            // needs a custom exception
            throw std::runtime_error{"could not parse argument for option " + key_ + ": '" + arg + "'"};
        }
    }

    // ... [snip] ...

private:
    std::string     key_;
    std::string     description_;
    std::any        argument_;
    std::type_index type_index_;
};

As you can see, the checking is done right when you try to assign the argument in argument(); the earliest possible point. Also, when a default value is given, that value is kept… it is not converted to a string (which may be lossy).

parameter(const std::string& key, help, const std::string& description = "")
  : key_{key}
  , description_{description}
  , type_info_(typeid(help)) {
}

You take all the string arguments as const&… but they’re sink arguments, they’re actually going to keep a copy of the string. If you pass them as const&, that means they will make a copy every time… even when they don’t have to.

When you do:

auto p = parameter{"foo", cli::parameter::help{}, "desc"};

… two strings are constructed—one for “foo” and one for “desc”—and then the constructor is actually called, and both strings are copied, so two more strings are constructed.

If you take the arguments by value, like this:

parameter(std::string key, help, std::string description = {})
    : key_{std::move(key)}
    , description_{std::move(description)}
    // ...

… then in the code example above, two strings are constructed—“foo” and “desc”—and then the constructor is called… but this time, the two strings are moved. Which is potentially much cheaper than a copy, and less likely to trigger an error.

Also, as a usability issue: is it really necessary to manually specify that you want a help message? Can we not assume that as a default, and then maybe offer a way to disable it or change the message for the rare cases that someone might want that?

    static std::string trim_dashs(const std::string& arg) {
      if (arg.size() > 2 && arg[0] == '-' && arg[1] == '-')
        return {arg.begin() + 2, arg.end()};
      if (arg.size() > 1 && arg[0] == '-')
        return {arg.begin() + 1, arg.end()};
      return arg;
    }

Does this really need to be part of the public interface of parameter?

Also, it looks like you could use a starts_with() function, to simplify:

auto trim_dashs(std::string const& arg)
{
    if (arg.size() > 2 and starts_with(arg, "--"))
        return arg.substr(2);
    if (arg.size() > 1 and starts_with(arg, '-'))
        return arg.substr(1);
    return arg;
}

starts_with() will be generally useful elsewhere, too.

    static bool is_valid_key(const std::string& in) {
      return (in.size() == 2 && in[0] == '-' && in[2] >= 'A' && in[2] <= 'z') ||
             (in.size() >= 3 && in[0] == '-' && in[1] == '-' && in[2] >= 'A' && in[2] <= 'z');
    }

There are a couple of issues here.

The first is that you have a bug. I think you meant to use in[1], not in[2] in the last part of the first line of that long expression.

Another issue is the way you’re checking for A <= ? <= z… that really doesn’t make a lot of sense. If you’re using Unicode, then you’re saying that -[ is a valid option… but -1 isn’t. That’s a little weird.

There doesn’t seem to be a reason why this function is part of parameter. It’s only used in cli, and only to check whether the next command line argument is (using regex) -[^-] or --.+.

    std::string key() const {
      return key_;
    }

    std::string description() const {
      return description_;
    }

These functions can be made much more efficient and no-fail (noexcept) by returning string const& rather than string.

    template <typename T>
    T as() const {
      assert((typeid(T).hash_code() == type_info_.get().hash_code()) ||
             ((typeid(T).hash_code() == typeid(bool).hash_code()) &&
              (typeid(parameter::single).hash_code() == type_info_.get().hash_code())));
      T arg;
      std::stringstream ss(argument_);
      ss >> arg;
      return arg;
    }

I already gave my thoughts on this function above, but I’ll just add that you don’t do any error checking on the conversion (ss >> arg).

    template <typename T = void>
    bool is() const {
      return typeid(T).hash_code() == type_info_.get().hash_code() ||
             typeid(T).hash_code() == typeid(parameter::value<void>).hash_code();
    }

What exactly is this function supposed to be checking? If this parameter is supposed to be int, then p.is<int>() returns true… but p.is<parameter::value<void>>() also returns true. What does that mean?

std::reference_wrapper<const std::type_info> type_info_;

This is a bit clunky and silly; it makes more sense to just store a std::type_index.

cli(const std::set<parameter> parameters, int argc, char** argv)

Before I get into the guts of this constructor, I want to note that I’m not keen on the idea of storing the parameter list in a set. (Or an unordered_set for that matter.)

The reason why is because very often the order of parameters is relevant. For example, if I have options for turning verbosity on and for “silent mode”, I might make the last one on the command line take precedence. Even if that’s not important, I may have ordered the options in some logical way so that when the help message is printed, they make more sense. Putting the parameters in a set (or unordered_set) throws away my ordering.

Now, you may argue “but set lookup is fast!”… but is it? You should try profiling that. You may be surprised.

In practice, your program is not going to have a million options. It probably won’t even have 100. The set of options is so small, that you probably won’t notice anything by leaving them “unordered” and searching through them sequentially.

Now, this only applies to the option description set: the list you create when you do:

{
    {"config", cli::parameter::value<std::string>(), "default configuration", std::string("Debug")},
    {"install", cli::parameter::single(), "build install target"},
    {"pack", cli::parameter::single(), "build package target"},
    {"timeout", cli::parameter::value<uint16_t>(), "timeout in seconds", uint16_t(10)}
}

The set you create after you actually read the options—what you call parameters_ in this class—doesn’t need to be ordered. At least I can’t see any reason why it would. And that could use fast lookup, because you only describe the options (as above) once, and you only parse the command line once (presumably), but you may access the parsed options many times.

So I would recommend creating a simple container class, maybe called parameters, that just holds a sequence of parameters in the order given:

auto const params = parameters{
    {"config", cli::parameter::value<std::string>(), "default configuration", std::string("Debug")},
    {"install", cli::parameter::single(), "build install target"},
    {"pack", cli::parameter::single(), "build package target"},
    {"timeout", cli::parameter::value<uint16_t>(), "timeout in seconds", uint16_t(10)}
};

parameters could be a bespoke class, or it could be as simple as:

using parameters = std::vector<parameter>;

Now the biggest problem with this constructor is that it really shouldn’t be a constructor. You are not merely constructing a cli object (whatever that means; but we’ll get back to that). You are doing multiple tasks. At the very least, you are parsing the command line and constructing a cli object with the results of the parse.

Shoehorning all that into a single constructor is bad form. It’s also inflexible. Let’s say I want to read program options from a configuration file instead of the command line. Or let’s say I want to read them from the command line and a configuration file. Now what? You’d have to completely redesign the entire class, over and over again, to handle every new use case. That’s a sign that you’ve messed up the abstraction.

Without thinking at all about implementation, this is the kind of thing I think of when I want a command-line argument system:

// i want to be able to declare all my options, and their format:
auto const cmd_line_params = cli::parameters{
    {"config",  cli::value<std::string> + cli::with_default("Debug"),   "default configuration"},
    {"install",                                                         "build install target"},
    {"pack",                                                            "build package target"},
    {"timeout", cli::with_default<uint16_t>(10),                        "timeout in seconds"} // cli::value<std::uint16_t> can be deduced

    // could also have other stuff, like:
    // {"required-option", cli::required, "this option is required"}
    // {"validated-option", cli::validate([](auto&& opt { return is_valid(opt); }), "this option is validated by a lambda"}
};

// i want to be able to declare *multiple* sets of options, and
// and combine them in various ways:
auto const secret_experimental_params = cli::parameters{
    // ...
};

// the above don’t need to be global args; they could be returned from
// functions, or hidden in a translation unit, or whatever
//
// ideally, in C++20 and beyond, all of the above could be constinit

// Lippincott function
auto handle_error() -> int
{
    try
    {
        throw;
    }
    catch (cli::option_error const& x)
    {
        std::cerr << x.what() << '\n'
        // print usage/help message, possibly using the parameters above
    }
    // etc.

    return EXIT_FAILURE;
}

auto main(int argc, char* argv) -> int
{
    try
    {
        auto const options = [argc, argv]
        {
            // first we try parsing some config files:
            auto opts = cli::parse(cmd_line_params, std::filesystem::path{"/etc/config"});

            // then we read options from the command line, which take precedence:
            opts.override_with(cli::parse(cmd_line_params, argc, argv));

            // or maybe with secret extra params, and codependent validation:
            // opts.override_with(cli::parse(
            //      cmd_line_params + secret_experimental_params,
            //      argc,
            //      argv,
            //      [] (auto&& opts) { check_options_1(opts); },
            //      [] (auto&& opts) { check_options_2(opts); }));

            return opts;
        }();

        // if there were *any* problems parsing the command line arguments, an
        // error would have been thrown... so if we got here, we know the
        // command line arguments are all good.

        std::cout << "this should never be a problem (normally): "
            << options["timeout"].as<std::uint16_t>()
            << '\n';
    }
    catch (...)
    {
        return handle_error();
    }
}

Basically, I’m suggesting separating the parsing logic from the parsing rules and actual storage of the parsed results… rather than cramming all three into a single class, as you have. Separating everything will allow much more flexibility, at every level.

  cli(const std::set<parameter> parameters, int argc, char** argv) 
    : command_line_{parameters} {

I don’t see the sense in declaring parameters as const, and then immediately just putting it command_line_. Why not leave parameters as non-const, and move it into command_line_?

    std::vector<std::string> argvector;
    for (int i = 1; i < argc; ++i) {
      argvector.push_back(argv[i]);
    }

Okay, first of all, if you’re going to pack all the arguments into a vector of strings, you might as well do it efficiently. At the very least you could do:

    std::vector<std::string> argvector;
    argvector.reserve(argc - 1); // reserve the needed memory
    for (int i = 1; i < argc; ++i) {
      argvector.push_back(argv[i]);
    }

But it would be easier to just do:

auto argvector = std::vector<std::string>(argv + 1, argv + argc);

Doing it that way allows the vector to resize itself once, rather than having to adjust itself as more arguments are added.

However… do you really need to copy the entire argument list into a vector, and convert each one to a string? Why can’t you just read them where they are. (Granted, this would be much easier with std::string_view, but, yeah, you’re stuck with C++14. You could always re-implement std::string_view, I suppose; it is super useful.)

    auto is_defined = [this](const std::string& in) {
      return command_line_.find({in, parameter::single()}) != command_line_.end();
    };

Yikes, this is wildly inefficient. To find a particular option, you’re constructing an entire dummy parameter object… and those things ain’t cheap! (You have to construct three strings, plus do a typeid() lookup.) But all you’re really doing is searching for a matching key:

auto is_defined = [&command_line_](auto&& in)
{
    return std::find_if(command_line_.begin(), command_line_.end(), [&in](auto&& p) { return p.key() == in; }) != command_line_.end();
};

Even if command_line_ is a set (and, remember, I don’t think it should be), this should still be significantly faster… even up to many, many hundreds of thousands of parameters.

      auto key = parameter::trim_dashs(*it);
      if (!parameter::is_valid_key(*it) || !is_defined(key)) {
        throw std::runtime_error(std::string("Unknown parameter '") + key + std::string("'\n") + help_message());
      }

      parameter p = *command_line_.find({key});

Okay, here’s the thing. First you construct the key as a string by trimming the dashes… and then you check whether it’s valid in the first place. But if it’s invalid, you’ve wasted the effort to construct it. And then, in is_defined(), you search through command_line_ to find the key… and if you find it, you just… throw that information away… and then search through command_line_ for the same key AGAIN.

Let’s rethink what’s going on here. First, you could really use std::string_view (or rather, you’ll have to re-implement it yourself in C++14, but that’s not hard). Constructing strings willy-nilly is painfully slow. That’s why std::string_view was created in the first place.

So to start, rather than creating a vector of strings, let’s just iterate through the command line arguments directly, creating string views for each one (which is cheap), and do all the checking one time rather than repeatedly:

for (auto p_arg = argv + 1; p_arg < argv + argc; ++p_arg)
{
    // parse_key() helper function tries to remove leading dashes and return
    // a string view of the key, but if the argument is malformed, throws an
    // exception
    auto const key = parse_key(*p_arg);

    auto const& param = [&command_line_](auto&& key)
    {
        auto const p = std::find_if(command_line_.begin(), command_line_.end(),
            [&key](auto&& p) { return p.key() == key; });

        if (p == command_line_.end())
            throw ...;

        return *p;
    }(key);

    // ...
}

Next, you check for a help option:

      if (p.is<parameter::help>()) {
        std::cerr << help_message() << std::endl;
        exit(0);
      }

The first problem with this is that when someone asks for help, they won’t expect it to be an error message. If I ask for help, that’s the expected output; it should be printed to std::cout if anything. But even better would be if I could decide where to print it.

But the real problem here is std::exit(). Never, ever, EVER use exit() in a C++ program. exit() is a C function; it doesn’t apply to C++. While some effort has been taken to make it sorta-kinda work with C++, the hard truth is that destructors for the vast majority of objects in most programs won’t be called… which could be catastrophic. Basically, you’re crashing the program, and who knows what state that will leave the environment in.

So what should you do here? Well, frankly, you’ve kinda painted yourself into a corner. Because this is a constructor, there are only two options for leaving it early: either crash the program (as with exit()), or throw an exception. You could make a special class like struct help_message_t {}; and throw that, and then catch it in main() to display a help message there, I suppose.

On the other hand, if you were doing the parsing with a function and not a constructor—which just makes sense conceptually anyway, because parsing is not constructing—then you could return a flag or something to indicate that the help message was requested, and let the calling code decide whether, how, and where to display it.

Next, you do this weird, back-assward check for an argument:

      auto next_it = std::next(it);
      const bool is_valid_argument = next_it != argvector.end() && !parameter::is_valid_key(*next_it);
      if (p.is<parameter::value<>>()) {
        if (is_valid_argument) {
          p.argument(*next_it);
          std::advance(it, 1);
        } else {
          throw std::runtime_error(std::string("Missing argument for '") + key + std::string("'\n"));
        }
      } else {
        p.argument("true");
      }

Rather than just forging ahead and parsing the next command line token and then checking to see if the option even has an argument, it makes more sense to check whether you even need an argument first:

    if (param requires an argument) // you'll need this check, somehow; the is() function currently makes no sense
    {
        if (++p_arg == argv + argc)
            throw ...;

        auto const arg = std::string_view{p_arg};

        // there's some weirdness going on here that i'll get into later.
        //
        // for now, this is basically what has to happen:
        parameters_[key] = convert_to<T>(arg);
        // convert_to<T>() just uses an istringstream to try to parse the
        // string arg as a T, and throws an exception if that fails
    }
    else
    {
        parameters_[key] = true;
    }

Now, there’s some weirdness with cli. It has two data members: command_line_ and parameters_. Normally I would assume that command_line_ is just the set of parameters passed in to the constructor, and parameters_ is what got parsed on the command line. But… so far as I can tell, that’s not what’s going on. Instead, command_line_ is used both for the options description and for the parsed options values. And parameters_ is used for nothing. But then, I see this:

  const parameter& operator[] (const std::string& key) {
    if (parameters_.count({key}))
      return *parameters_.find({key});
    auto p = command_line_.find({key});
    return *p;
  }

What is going on there?

  std::string help_message() const {
    std::string message = "Usage: builder [options]\n"
                          "Allowed options:\n";
    for (auto& argument : command_line_) {
      message += std::string("\t--");
      message += argument.key();
      message += std::string(std::max(0, 10 - static_cast<int>(argument.key().size())), ' ');
      message += std::string(":");
      message += argument.description();
      message += std::string("\n");
    }

    return message;
  }

Returning the help message as a string isn’t bad… but more often than not, when you want the help message, it’s because you’re writing it to some stream. So why not make that possible:

auto write_usage_message(std::ostream& out) const
{
    // it would be nice to be able to customize this
    out << "Usage: builder [options]\n";
}

auto write_help_message(std::ostream& out) const
{
    write_usage_message(out);

    out << "Allowed options:\n";

    for (auto&& parameter : command_line_)
    {
        out << "\t--" << parameter.key();

        for (indent as you please)
            out.put(' ');

        out << ':' << parameter.description() << '\n';
    }
}

If you want you could even still have your help_message() function:

auto help_message() const
{
    auto oss = std::ostringstream{};
    write_help_message(oss);
    return oss.str();
}

And it would incidentally probably be more efficient than your existing help_message() function.

I think you need to rethink the overall design, because you have a single class that handles far too much—parsing, as well as storing the parsed data—not to mention that it does all of that in the constructor.

Perhaps a better design would be basically 3 things:

  1. A class to describe options in a way that tells the parser how options should be parsed (basically parameter, but stripped down a bit—like, don’t actually store the parsed value).
  2. A class to hold parsed options (basically a map where the key is the option name, and the value is a std::any with the parsed value, if any… but of course, not actually std::any, but rather your own C++14 version of it).
  3. A parser function that basically takes (1) as argument, and returns (2).

The tricky part is how to get an actual parse function for T from the type_info stored in the options description. That will be challenging… but not impossible. And it will be much better than just storing the value as a string and then parsing it on demand, both in terms of usability and efficiency.

\$\endgroup\$
0
2
\$\begingroup\$

Here are some things that may help you improve your code.

Provide a way for users to catch errors

The cli constructor can throw an exception, which is not necessarily bad, but the lack of any other constructors make it hard to use. Consider this snippet as an attempt to use your library:

cli c;
try {
    c = cli{parameters, argc, argv};
} catch (std::runtime_error &err) {
    std::cerr << err.what() << '\n';
    return 1;
} 
auto bar{c["timeout"].as<uint16_t>()};
std::cout << "timeout = " << bar << '\n';

This fails because there is no default non-throwing constructor and no copy constructor. If we try to move the declaration inside the try block, it will fail again because now the use of c after the catch block attempts to use a variable no longer in scope. I would suggest adding these two lines:

cli() = default;
cli(const cli& other) = default;

However, I haven't checked thoroughly to make sure that doesn't introduce other problems. You should.

Be consistent about throw vs assert

If we attempt to call this as builder --timeout 17 --install /usr/bin the code aborts on a failed assert. I'd expect that a better approach would be to throw to allow the user a chance to handle the error gracefully.

Avoid += for strings

The help_message() code, among other places uses operator+= to concatenate strings in many places. There are a few problems with this. The first is that each invocation of the operator may require memory reallocation for the resulting std::string which slows things down and may fragment memory. The second is that the string is built every time that function is called -- it could instead be built by the constructor since it does not change.

Consider alternatives to std::set

I don't see a reason the code couldn't use std::unordered_set instead of std::set. Using std::unordered_set often confers a performance advantage over std::set if you don't need ordering.

Think of the user

Consider this line in the code listed above:

auto bar{c["timeout"].as<uint16_t>()};

As a user of the code, I might reasonably wonder why I can't just write this:

auto bar{c["timeout"]};

What this actually does is return the exact same cli::parameter that the user passed in to construct the cli, but isn't it more likely that what the user wants the associated value instead? This might be easier with something like the C++17 type_identity.

Clearly separate the interface

The interface includes the required #includes. I believe the ones this needs are:

#include <cassert>
#include <functional>
#include <iostream>
#include <set>
#include <stdexcept>
#include <string>
#include <sstream>
#include <typeinfo>
#include <vector>
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.