Matthew Herrmann wrote:

...

I tried using

bpo::options_description desc("Allowed options"); desc.add_options() (HELP, "produce help message");

but this fails to compile because HELP is an std::string.

Isn't that a bit too restrictive? Last I checked, boost::program_options took const char* in several

Thorsten Ottosen wrote: places where it would end up storing them as std::strings anyway. Taking "const std::string&" would also clarify that passing temporary strings is fine, whereas "const char*" as an arg hints to me that the pointers should remain valid until options_description is destroyed (purely question of taste though).

I'd vote to replace const char* everywhere in the program options interface with std::string. (excluding the "char* argv[]" of course)

'replace'? I'm afraid, over my dead body only. I surely don't want implicit conversion to happen, and bloat the code, for every name and description of the option. I don't recall why string overloads are not provided, I'll have to think about it. - Volodya

Hi, ----- Original Message ----- From: "Phil Richards" <news@derived-software.ltd.uk> To: <boost@lists.boost.org> Sent: Saturday, March 06, 2010 11:59 AM Subject: Re: [boost] [program_options]

On Fri, 2010-03-05 at 09:21 +0300, Vladimir Prus wrote:

...

Matthew Herrmann wrote:

...

I'd vote to replace const char* everywhere in the program options interface with std::string. (excluding the "char* argv[]" of course) 'replace'? I'm afraid, over my dead body only. I surely don't want implicit conversion to happen, and bloat the code, for every name and description of the option.

Well, I certainly don't want to see anybody's dead body, but here is the code for setting name:

option_description::set_name(const char* _name) { std::string name(_name); string::size_type n = name.find(','); if (n != string::npos) { assert(n == name.size()-2); m_long_name = name.substr(0, n); m_short_name = '-' + name.substr(n+1,1); } else { m_long_name = name; } return *this; }

I can't see how taking a std::string const& as an argument has any detrimental effect on conversions or code bloat. The std::string version of set_name would just remove the first line of the function (creating the temporary "name"), and use _name throughout the function body.

Description is even simpler: it just uses the const char* argument as a parameter to a std::string constructor. Accepting a std::string const& instead of a const char* will have minimal overheads (possibly one extra temporary string creation).

But maybe I'm missing something obvious?

The fact that currently the implementation use a temporary string doesn't means that we can not change the implementation. The interface with a string const reference involves already an allocation and deallocation when you have a const char*. What about having both overloads? const char * is the best for literals, while string cont& is the best for strings variables. void option_description::set_name(string const&_name); void option_description::set_name(const char* _name); If the current implementation uses a temporary string, it needs just to forward the call. void option_description::set_name(const char* _name) { std::string name(_name); option_description::set_name(name); } Just my 2cts, Vicente

On 03/06/2010 10:03 PM, Phil Richards wrote:

My point was that there is no need for having both overloads: it doesn't (as far as I can see) provide any benefit having an "efficient" const char* call - there isn't any (obvious) extra conversions or code bloat by providing just the const std::string& interface because internally the const char* are immediately converted to std::string.

But, as I said before, I might be missing something.

I didn't analyze the code deeply but the code bloat may take place because the function gets called from user's code multiple times. This means that the literal -> string conversion is generated many times in user's code instead of being generated once in the compiled binary of Boost.ProgramOptions. Again, I didn't ensure that is the case.

vicente.botet wrote:

Hi, ----- Original Message ----- From: "Phil Richards" <news@derived-software.ltd.uk> To: <boost@lists.boost.org> Sent: Saturday, March 06, 2010 11:59 AM Subject: Re: [boost] [program_options]

...

On Fri, 2010-03-05 at 09:21 +0300, Vladimir Prus wrote:

...

Matthew Herrmann wrote:

...

I'd vote to replace const char* everywhere in the program options interface with std::string. (excluding the "char* argv[]" of course) 'replace'? I'm afraid, over my dead body only. I surely don't want implicit conversion to happen, and bloat the code, for every name and description of the option.

Well, I certainly don't want to see anybody's dead body, but here is the code for setting name:

option_description::set_name(const char* _name) { std::string name(_name); string::size_type n = name.find(','); if (n != string::npos) { assert(n == name.size()-2); m_long_name = name.substr(0, n); m_short_name = '-' + name.substr(n+1,1); } else { m_long_name = name; } return *this; }

I can't see how taking a std::string const& as an argument has any detrimental effect on conversions or code bloat. The std::string version of set_name would just remove the first line of the function (creating the temporary "name"), and use _name throughout the function body.

Sorry, you have said 'replace' -- which means changing const char* to std::string (const, &, whatever). That would meant creating a string at each call site where you pass const char*, which is more code than required to just push pointer to the stack.

...

Description is even simpler: it just uses the const char* argument as a parameter to a std::string constructor. Accepting a std::string const& instead of a const char* will have minimal overheads (possibly one extra temporary string creation).

But maybe I'm missing something obvious?

The fact that currently the implementation use a temporary string doesn't means that we can not change the implementation. The interface with a string const reference involves already an allocation and deallocation when you have a const char*.

What about having both overloads? const char * is the best for literals, while string cont& is the best for strings variables.

void option_description::set_name(string const&_name); void option_description::set_name(const char* _name);

As I've said already, I don't remember why this is not provided, and will have to think about it. - Volodya

On Sun, 2010-03-07 at 09:25 +0300, Vladimir Prus wrote:

...

From: "Phil Richards" <news@derived-software.ltd.uk>

...

I can't see how taking a std::string const& as an argument has any detrimental effect on conversions or code bloat. The std::string version of set_name would just remove the first line of the function (creating the temporary "name"), and use _name throughout the function body. Sorry, you have said 'replace' -- which means changing const char* to std::string (const, &, whatever). That would meant creating a string at each call site where you pass const char*, which is more code than required to just push pointer to the stack.

Well, OK, if you are concerned about overheads at that level, on a piece of code that is generally going to be invoked once immediately after program start-up, then I don't think I've got any counter-argument. I just can't actually see that it is a big issue, but there are obviously use-cases where it must be. So I'll withdraw "replace" and say "additionally". Phil -- Phil Richards, <news@derived-software.ltd.uk>

"vicente.botet" <vicente.botet@wanadoo.fr> wrote in message news:06810FA68C344871BDFED63A5595AF0D@viboes1... ...

What about having both overloads? const char * is the best for literals, while string cont& is the best for strings variables.

void option_description::set_name(string const&_name); void option_description::set_name(const char* _name); ...

Well, looking at the implementation of set_name() posted above: option_description::set_name(const char* _name) { std::string name(_name); string::size_type n = name.find(','); if (n != string::npos) { assert(n == name.size()-2); m_long_name = name.substr(0, n); m_short_name = '-' + name.substr(n+1,1); } else { m_long_name = name; } return *this; } it can be noticed that it, in the case where a comma is present in the string, makes 4 to 6 allocations (and copies) when it should make 0 to 2...and in the 'other' case makes 1 or 2 allocations when it should make 0 or 1... If we change the implementation as follows: typedef iterator_range<char const *> string_chunk_t; option_description::set_name( string_chunk_t const name ) { string_chunk_t::const_iterator const pComma( std::find( name.begin(), name.end(), ',' ) ); if ( pComma != name.end() ) { assert( pComma == name.end() - 2 ); char const short_name[ 2 ] = { '-', name.back() }; m_short_name.assign( boost::begin( short_name ), boost::end( short_name ) ); } m_long_name.assign( name.begin(), pComma ); return *this; } we solve both the inefficency issues outlined above as well as the char const * / std::string const & overload issues. It will even eliminate the strlen() calls for literals. Unfortunately, because of certain issues with boost::range (that I will go into more detail in a separate post) the following two overloads are currently also necessary: template <class Range> option_description::set_name( Range const & string_range, typename disable_if<is_pointer<Range>>::type * = 0 ) { char const * const begin( &* boost::begin( string_range ) ); char const * const end ( ( &*( boost::end ( string_range ) - 1 ) ) + 1 ); assert( is_array<Range>::value == ( *(end - 1) == '\0' ) ); set_name( string_chunk_t( begin, is_array<Range>::value ? end - 1 : end ) ); } template <class Range> option_description::set_name( Range const & name, typename enable_if<is_pointer<Range>>::type * = 0 ) { set_name( boost::as_literal( name ) ); } ps. "From the looks of that assertion" it seems the suggested implementation can be further improved by eliminating even the std::find() operation: option_description::set_name( string_chunk_t const name ) { string_chunk_t::const_iterator pComma( name.end() ); if ( *( name.end() - 2 ) == ',' ) { pComma = name.end() - 2; char const short_name[ 2 ] = { '-', name.back() }; m_short_name.assign( boost::begin( short_name ), boost::end( short_name ) ); } else assert( std::find( name.begin(), name.end(), ',' ) == name.end() ); m_long_name.assign( name.begin(), pComma ); return *this; } Now, if this is necessary, the strong exception safety guarantee could be provided by first calling reserve() on m_long_name at the beginning of the function.... -- "What Huxley teaches is that in the age of advanced technology, spiritual devastation is more likely to come from an enemy with a smiling face than from one whose countenance exudes suspicion and hate." Neil Postman

Matthew Herrmann <matthew.herrmann <at> zomojo.com> writes:

Thorsten Ottosen wrote:

...

I tried using

bpo::options_description desc("Allowed options"); desc.add_options() (HELP, "produce help message");

but this fails to compile because HELP is an std::string.

I'd vote to replace const char* everywhere in the program options interface with std::string. (excluding the "char* argv[]" of course)

This would certainly help for cases where I construct the description string from other program details, i.e. it would avoid having to extract the "c_str ()", e.g. std::string otherProgramDetail; add_opt... ("option", ("blah blah " + otherProgramDetail + " more blah").c_str() ) Also: - The name/description are held as strings internally, anyway. - The "default_value()"'s second argument, for specifying the default value as a textual representation, _is_ an std::string. So it would improve consistency. So, +1. Regards, Chard.