[Spice-devel] [PATCH spice 5/9] spicec: enable multiple CmdLineParser instantiations

[Hans de Goede]

Hi,

On 10/17/2010 04:32 PM, Hans de Goede wrote:
> Ack.
>

Erm, too soon, nack, ...

> On 10/17/2010 04:13 PM, Arnon Gilboa wrote:
>> Used by controller. One instance at a time, not thread-safe.
>> Add basename() for win32.
>> ---
>> client/cmd_line_parser.cpp | 23 ++++++++++++++++++++++-
>> 1 files changed, 22 insertions(+), 1 deletions(-)
>>
>> diff --git a/client/cmd_line_parser.cpp b/client/cmd_line_parser.cpp
>> index 3f45551..a9c1857 100644
>> --- a/client/cmd_line_parser.cpp
>> +++ b/client/cmd_line_parser.cpp
>> @@ -53,6 +53,11 @@ CmdLineParser::CmdLineParser(std::string description, bool allow_positional_args
>> , _positional_args (allow_positional_args)
>> , _done (false)
>> {
>> + //Enables multiple instantiations. One at a time, not thread-safe.
>> + optind = 1;
>> + opterr = 1;
>> + optopt = 0;
>> + optarg = 0;
>> }
>>
>> CmdLineParser::~CmdLineParser()
>> @@ -428,13 +433,29 @@ char* CmdLineParser::next_argument()
>> return _argv[optind++];
>> }
>>
>> +#ifdef WIN32
>> +char* basename(char *str)
>> +{
>> + char *base;
>> + if ((base = strrchr(str, '\\'))) {
>> + return base;
>> + }
>> +
>> + if ((base = strrchr(str, ':'))) {
>> + return base;
>> + }
>> + return str;
>> +}
>> +
>> +#endif
>> +

UGLY is this really necessary? Does windows insists
on putting the full path in argv[0]? even when executed
from a cmd line with the executable in the searchpath ?

And even if it does, this just affects how the help
is printed for cmdline use, hardly worth adding an
#ifdef for IMHO.

Also doing the basename() will break compilation on other
platforms (Mac OS X), which is why Alex removed it. and
it is wrong on Linux too, the help should show
/some/custom/path/spicec if that is how the client was
launched (and unix will pass in just spicec if it was
launched as spicec and found as such in the PATH).

>> void CmdLineParser::show_help()
>> {
>> static const int HELP_START_POS = 30;
>> static const int HELP_WIDTH = 80 - HELP_START_POS;
>> std::ostringstream os;
>>
>> - os<< _argv[0]<< " - "<< _description.c_str()<< "\n\noptions:\n\n";
>> + os<< basename(_argv[0])<< " - "<< _description.c_str()<< "\n\noptions:\n\n";
>>
>> Options::iterator iter = _options.begin();
>> for (; iter != _options.end(); ++iter) {

Regards,

Hans