Re: [Boost-users] [program_options] patch - optional argument support (gnu getopt_long style)

From: Vladimir Prus (ghost_at_[hidden])
Date: 2007-08-07 14:26:07

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<T, charT>::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<std::basic_string<charT> >& 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