Bryan Green wrote:
Attached is a patch to program_options that implements optional argument
support, in the style of GNU getopt_long. It adds one new method to
'value_type': 'implicit_value()'.
The idea is that, if the option is given without an argument, it is
assigned
an "implicit" value. Otherwise, if the long-option is given an explicit
argument via the equals sign (--arg=value), or the short-option is given
an adjacent argument (-avalue), the explicit value will override the
implicit one.
Hi Bryan,
thanks for the patch. I have some comments about it:
1. I'd appreciate doxygen comments for the 'implicit_value'
methods you've added, so that generated docs say something
about those.
2. In value_semantics.hpp, you have this:
- if (!m_default_value.empty() && !m_default_value_as_text.empty()) {
+ if (!m_implicit_value.empty() && !m_implicit_value_as_text.empty()) {
+ return "[=arg] (=" + m_implicit_value_as_text + ")";
+ }
So apparently, if implicit value is provided, the default value is
not shown? Is this intended, and why is this good?
3. In this code:
void
typed_value::notify(const boost::any& value_store) const
{
- const T* value = boost::any_cast<const T>(&value_store);
+ const T* value;
+ if (value_store.empty())
+ value = boost::any_cast<const T>(&m_implicit_value);
+ else
+ value = boost::any_cast<const T>(&value_store);
if (m_store_to) {
*m_store_to = *value;
}
why value_store can end up being empty? I would expect that the xparse
method, in the event that no explicit value is provided, would store
m_implicit_value in value_store. The way you do this, it seems that
the right value will be stored by 'notify', but the value stored
to variables_map will be wrong.
4. In this code:
xparse(boost::any& value_store,
const std::vector& new_tokens) const
{
- validate(value_store, new_tokens, (T*)0, 0);
+ if (!new_tokens.empty() || m_implicit_value.empty())
+ validate(value_store, new_tokens, (T*)0, 0);
}
I'd appreciate a comment. It took me a couple of minutes to understand
that the logic is indeed right.
5. For
+ // A value is optional if min_tokens == 0, but max_tokens > 0.
+ // If a value is optional, it must appear in opt.value (because
+ // it was 'adjacent'. Otherwise, remove the expectation of a
+ // non-adjacent value.
+ if (min_tokens == 0 && max_tokens > 0 && opt.value.empty())
+ --max_tokens;
+
what will happen if in future, there's some option that accepts [0,10]
tokens. Your code will make it access [0,9] tokens, IIUC. Maybe you should
explicitly check for max_tokens == 1?
6. I'd surely appreciate a test ;-)
Do you think you can adjust the patch per above? Clearly, I'd need to code questions
to be settled before applying this. I'd very much like test/comment changes
too, but if you're out of time we can skip them.
Thanks,
Volodya
