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