[libcamera-devel] [PATCH 09/30] cam: options: Support parent-child relationship between options
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jul 12 16:12:28 CEST 2021
On 07/07/2021 03:19, Laurent Pinchart wrote:
> Add support for creating a tree-based hiearchy of options instead of a
s/hiearchy/hierarchy/
> flat list. This is useful to support options that need to be interpreted
> in the context of a particular occurrence of an array option.
How is this expressed in the help? (i.e.l how does the user know which
ones establish a new parent set of options).
An example output up here would be useful if it changed.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/cam/options.cpp | 217 ++++++++++++++++++++++++++++++++++++++------
> src/cam/options.h | 14 ++-
> 2 files changed, 199 insertions(+), 32 deletions(-)
>
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 4a88c38fb154..d7f9f741c731 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -79,6 +79,12 @@
> * \var Option::isArray
> * \brief Whether the option can appear once or multiple times
> *
> + * \var Option::parent
> + * \brief The parent option
> + *
> + * \var Option::children
> + * \brief List of child options, storing all options whose parent is this option
> + *
> * \fn Option::hasShortOption()
> * \brief Tell if the option has a short option specifier (e.g. `-f`)
> * \return True if the option has a short option specifier, false otherwise
> @@ -96,6 +102,8 @@ struct Option {
> const char *help;
> KeyValueParser *keyValueParser;
> bool isArray;
> + Option *parent;
> + std::list<Option> children;
>
> bool hasShortOption() const { return isalnum(opt); }
> bool hasLongOption() const { return name != nullptr; }
> @@ -336,7 +344,7 @@ bool KeyValueParser::addOption(const char *name, OptionType type,
> return false;
>
> optionsMap_[name] = Option({ 0, type, name, argument, nullptr,
> - help, nullptr, false });
> + help, nullptr, false, nullptr, {} });
> return true;
> }
>
> @@ -473,6 +481,11 @@ void KeyValueParser::usage(int indent)
> * option. It supports empty values, integers, strings, key-value lists, as well
> * as arrays of those types. For array values, all array elements shall have the
> * same type.
> + *
> + * OptionValue instances are organized in a tree-based structure that matches
> + * the parent-child relationship of the options added to the parser. Children
> + * are retrieve with the children() function, and are stored as an
s/retrieve/retrieved/
> + * OptionsBase<int>.
> */
>
> /**
> @@ -663,6 +676,15 @@ std::vector<OptionValue> OptionValue::toArray() const
> return array_;
> }
>
> +/**
> + * \brief Retrieve the list of child values
> + * \return The list of child values
> + */
> +const OptionsParser::Options &OptionValue::children() const
> +{
> + return children_;
> +}
> +
> /* -----------------------------------------------------------------------------
> * OptionsParser
> */
> @@ -725,6 +747,32 @@ std::vector<OptionValue> OptionValue::toArray() const
> * accept an argument, the option value can be access by Options::operator[]()
> * using the option identifier as the key. The order in which different options
> * are specified on the command line isn't preserved.
> + *
> + * Options can be created with parent-child relationships to organize them as a
> + * tree instead of a flat list. When parsing a command line, the child options
> + * are considered related to the parent option that precedes them. This is
> + * useful when the parent is an array option. The Options values list generated
> + * by the parser then turns into a tree, which each parent value storing the
> + * values of child options that follow that instance of the parent option.
> + * For instance, with a `capture` option specified as a child of a `camera`
> + * array option, parsing the command line
> + *
> + * `--camera 1 --capture=10 --camera 2 --capture=20`
> + *
> + * will return an Options instance containing a single OptionValue instance of
> + * array type, for the `camera` option. The OptionValue will contain two
> + * entries, with the first entry containing the integer value 1 and the second
> + * entry the integer value 2. Each of those entries will in turn store an
> + * Options instance that contains the respective children. The first entry will
> + * store in its children a `capture` option of value 10, and the second entry a
> + * `capture` option of value 20.
> + *
> + * The command line
> + *
> + * `--capture=10 --camera 1`
> + *
> + * would result in a parsing error, as the `capture` option has no preceding
> + * `camera` option on the command line.
> */
>
> /**
> @@ -748,13 +796,14 @@ OptionsParser::~OptionsParser() = default;
> * mandatory argument, or no argument at all
> * \param[in] argumentName The argument name used in the help text
> * \param[in] array Whether the option can appear once or multiple times
> + * \param[in] parent The identifier of the parent option (optional)
> *
> * \return True if the option was added successfully, false if an error
> * occurred.
> */
> bool OptionsParser::addOption(int opt, OptionType type, const char *help,
> const char *name, OptionArgument argument,
> - const char *argumentName, bool array)
> + const char *argumentName, bool array, int parent)
> {
> /*
> * Options must have at least a short or long name, and a text message.
> @@ -771,9 +820,31 @@ bool OptionsParser::addOption(int opt, OptionType type, const char *help,
> if (optionsMap_.find(opt) != optionsMap_.end())
> return false;
>
> - options_.push_back(Option({ opt, type, name, argument, argumentName,
> - help, nullptr, array }));
> - optionsMap_[opt] = &options_.back();
> + /*
> + * If a parent is specified, create the option as a child of its parent.
> + * Otherwise, create it in the parser's options list.
> + */
> + Option *option;
> +
> + if (parent) {
> + auto iter = optionsMap_.find(parent);
> + if (iter == optionsMap_.end())
> + return false;
> +
> + Option *parentOpt = iter->second;
> + parentOpt->children.push_back({
> + opt, type, name, argument, argumentName, help, nullptr,
> + array, parentOpt, {}
> + });
> + option = &parentOpt->children.back();
> + } else {
> + options_.push_back({ opt, type, name, argument, argumentName,
> + help, nullptr, array, nullptr, {} });
> + option = &options_.back();
> + }
> +
> + optionsMap_[opt] = option;
> +
> return true;
> }
>
> @@ -791,13 +862,13 @@ bool OptionsParser::addOption(int opt, OptionType type, const char *help,
> * occurred.
> */
> bool OptionsParser::addOption(int opt, KeyValueParser *parser, const char *help,
> - const char *name, bool array)
> + const char *name, bool array, int parent)
> {
> if (!addOption(opt, OptionKeyValue, help, name, ArgumentRequired,
> - "key=value[,key=value,...]", array))
> + "key=value[,key=value,...]", array, parent))
> return false;
>
> - options_.back().keyValueParser = parser;
> + optionsMap_[opt]->keyValueParser = parser;
> return true;
> }
>
> @@ -822,26 +893,26 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> * Allocate short and long options arrays large enough to contain all
> * options.
> */
> - char shortOptions[options_.size() * 3 + 2];
> - struct option longOptions[options_.size() + 1];
> + char shortOptions[optionsMap_.size() * 3 + 2];
> + struct option longOptions[optionsMap_.size() + 1];
> unsigned int ids = 0;
> unsigned int idl = 0;
>
> shortOptions[ids++] = ':';
>
> - for (const Option &option : options_) {
> - if (option.hasShortOption()) {
> - shortOptions[ids++] = option.opt;
> - if (option.argument != ArgumentNone)
> + for (const auto [opt, option] : optionsMap_) {
> + if (option->hasShortOption()) {
> + shortOptions[ids++] = opt;
> + if (option->argument != ArgumentNone)
> shortOptions[ids++] = ':';
> - if (option.argument == ArgumentOptional)
> + if (option->argument == ArgumentOptional)
> shortOptions[ids++] = ':';
> }
>
> - if (option.hasLongOption()) {
> - longOptions[idl].name = option.name;
> + if (option->hasLongOption()) {
> + longOptions[idl].name = option->name;
>
> - switch (option.argument) {
> + switch (option->argument) {
> case ArgumentNone:
> longOptions[idl].has_arg = no_argument;
> break;
> @@ -854,7 +925,7 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> }
>
> longOptions[idl].flag = 0;
> - longOptions[idl].val = option.opt;
> + longOptions[idl].val = option->opt;
> idl++;
> }
> }
> @@ -882,10 +953,7 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> }
>
> const Option &option = *optionsMap_[c];
> - if (!options.parseValue(c, option, optarg)) {
> - std::cerr << "Can't parse " << option.typeName()
> - << " argument for option " << option.optionName()
> - << std::endl;
> + if (!parseValue(option, optarg, &options)) {
> usage();
> return options;
> }
> @@ -907,15 +975,16 @@ void OptionsParser::usage()
> {
> unsigned int indent = 0;
>
> - for (const Option &option : options_) {
> + for (const auto &opt : optionsMap_) {
> + const Option *option = opt.second;
> unsigned int length = 14;
> - if (option.hasLongOption())
> - length += 2 + strlen(option.name);
> - if (option.argument != ArgumentNone)
> - length += 1 + strlen(option.argumentName);
> - if (option.argument == ArgumentOptional)
> + if (option->hasLongOption())
> + length += 2 + strlen(option->name);
> + if (option->argument != ArgumentNone)
> + length += 1 + strlen(option->argumentName);
> + if (option->argument == ArgumentOptional)
> length += 2;
> - if (option.isArray)
> + if (option->isArray)
> length += 4;
>
> if (length > indent)
> @@ -938,6 +1007,8 @@ void OptionsParser::usage()
> void OptionsParser::usageOptions(const std::list<Option> &options,
> unsigned int indent)
> {
> + std::vector<const Option *> parentOptions;
> +
> for (const Option &option : options) {
> std::string argument;
> if (option.hasShortOption())
> @@ -982,5 +1053,91 @@ void OptionsParser::usageOptions(const std::list<Option> &options,
>
> if (option.keyValueParser)
> option.keyValueParser->usage(indent);
> +
> + if (!option.children.empty())
> + parentOptions.push_back(&option);
> }
> +
> + if (parentOptions.empty())
> + return;
> +
> + for (const Option *option : parentOptions) {
> + std::cerr << std::endl << "Options valid in the context of "
> + << option->optionName() << ":" << std::endl;
> + usageOptions(option->children, indent);
> + }
> +}
> +
> +std::tuple<OptionsParser::Options *, const Option *>
> +OptionsParser::childOption(const Option *parent, Options *options)
> +{
> + /*
> + * The parent argument points to the parent of the leaf node Option,
> + * and the options argument to the root node of the Options tree. Use
> + * recursive calls traverse the Option tree up to the root node while
Use recursive calls "to" traverse the Option tree ?
I.e. is the to missing?
> + * traversing the Options tree down to the leaf node:
To traverse up while traversing down ... sounds ... odd ?
> + */
> +
> + /*
> + * - If we have no parent, we're reached the root node of the Option
s/we're/we've/
> + * tree, the options argument is what we need.
> + */
> + if (!parent)
> + return { options, nullptr };
> +
> + /*
> + * - If the parent has a parent, use recursion to move one level up the
> + * Option tree. This returns the Options corresponding to parent, or
> + * nullptr if a suitable Options child isn't found.
> + */
> + if (parent->parent) {
> + const Option *error;
> + std::tie(options, error) = childOption(parent->parent, options);
> +
> + /* Propagate the error all the way up. */
propagate up or down ? The previous comment says we're moving up a
level, so do we then propagate the error down? Or perhaps do you mean
'back up the callstack' instead...
Otherwise we have 'recurse up', and 'propagate up' which I think are
both different 'directions'...
> + if (!error)
> + return { options, error };
> + }
> +
> + /*
> + * - The parent has no parent, we're now one level down the root.
> + * Return the Options child corresponding to the parent. The child may
> + * not exists if options are specified in an incorrect order.
s/exists/exist/
Phew, ok - well this patch is terse, but I can't see anything
specifically other than those minors so:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> + */
> + if (!options->isSet(parent->opt))
> + return { nullptr, parent };
> +
> + /*
> + * If the child value is of array type, children are not stored in the
> + * value .children() list, but in the .children() of the value's array
> + * elements. Use the last array element in that case, as a child option
> + * relates to the last instance of its parent option.
> + */
> + const OptionValue *value = &(*options)[parent->opt];
> + if (value->type() == OptionValue::ValueArray)
> + value = &value->toArray().back();
> +
> + return { const_cast<Options *>(&value->children()), nullptr };
> +}
> +
> +bool OptionsParser::parseValue(const Option &option, const char *arg,
> + Options *options)
> +{
> + const Option *error;
> +
> + std::tie(options, error) = childOption(option.parent, options);
> + if (error) {
> + std::cerr << "Option " << option.optionName() << " requires a "
> + << error->optionName() << " context" << std::endl;
> + return false;
> + }
> +
> + if (!options->parseValue(option.opt, option, arg)) {
> + std::cerr << "Can't parse " << option.typeName()
> + << " argument for option " << option.optionName()
> + << std::endl;
> + return false;
> + }
> +
> + return true;
> }
> diff --git a/src/cam/options.h b/src/cam/options.h
> index 5c51a94c2f37..e894822c0061 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -10,6 +10,7 @@
> #include <ctype.h>
> #include <list>
> #include <map>
> +#include <tuple>
> #include <vector>
>
> class KeyValueParser;
> @@ -91,9 +92,11 @@ public:
> bool addOption(int opt, OptionType type, const char *help,
> const char *name = nullptr,
> OptionArgument argument = ArgumentNone,
> - const char *argumentName = nullptr, bool array = false);
> + const char *argumentName = nullptr, bool array = false,
> + int parent = 0);
> bool addOption(int opt, KeyValueParser *parser, const char *help,
> - const char *name = nullptr, bool array = false);
> + const char *name = nullptr, bool array = false,
> + int parent = 0);
>
> Options parse(int argc, char *argv[]);
> void usage();
> @@ -104,6 +107,10 @@ private:
>
> void usageOptions(const std::list<Option> &options, unsigned int indent);
>
> + std::tuple<OptionsParser::Options *, const Option *>
> + childOption(const Option *parent, Options *options);
> + bool parseValue(const Option &option, const char *arg, Options *options);
> +
> std::list<Option> options_;
> std::map<unsigned int, Option *> optionsMap_;
> };
> @@ -139,12 +146,15 @@ public:
> KeyValueParser::Options toKeyValues() const;
> std::vector<OptionValue> toArray() const;
>
> + const OptionsParser::Options &children() const;
> +
> private:
> ValueType type_;
> int integer_;
> std::string string_;
> KeyValueParser::Options keyValues_;
> std::vector<OptionValue> array_;
> + OptionsParser::Options children_;
> };
>
> #endif /* __CAM_OPTIONS_H__ */
>
More information about the libcamera-devel
mailing list