On 07/14/2014 07:02 AM, Amos Jeffries wrote:
> On 23/06/2014 12:35 a.m., Amos Jeffries wrote:
>> On 17/06/2014 4:06 a.m., Alex Rousskov wrote:
>>> On 06/15/2014 12:05 AM, Amos Jeffries wrote:
>>>> + if (optarg) {
>>>> + SBuf t(optarg);
>>>> + ::Parser::Tokenizer tok(t);
>>>
>>> Pleaser remove "t" as used-once and lacking a descriptive name. If you
>>> insist on keeping it, please make it "const" and try to give it a more
>>> descriptive name such as rawName or serviceName.
> No can do on removal. Without it we get:
> error: request for member 'prefix' in 'tok', which is of non-class type
> 'Parser::Tokenizer(SBuf)'
Yeah, we appear to hit some hairy C++ concept here that I cannot fully
reconstruct. I could not find an explanation by quick googling (all
results point to trivial cases of declaring a function instead of
calling the default constructor).
The following works for me:
::Parser::Tokenizer tok(SBuf(optarg, SBuf::npos));
However, at this point, it is not clear whether the extra "t" temporary
is actually worse than the extra SBuf argument! If you keep "t", a more
descriptive name for t would be nice but obviously not required.
>>>> + if (!tok.prefix(service_name, chr) || !tok.atEnd())
>>>
>>> Please note that the above also rejects empty service names (-n "").
>>> That may be good, but the error messages do not reflect that restriction.
>>>
>
> Moved this to an explicit check on *optarg content so it is obviously
> intentional and gets the right "missing service name" error.
The adjusted if condition in the committed code appears to dereference a
nil pointer when the service name is missing:
> if (optarg || *optarg == '\0') {
The code actually does not get that far because getopt() does not allow
a missing service name (at least in my tests), but the condition should
be fixed.
HTH,
Alex.
