Fix issue 14724 - std.getopt: config.required breaks --help

[burner]

https://issues.dlang.org/show_bug.cgi?id=14724

[schveiguy]

Doesn't this kill the tail-call optimization?

[DmitryOlshansky]

I don't see getopt being enough of bottleneck anywhere to care about micro-optimizations like tail-call here.

[schveiguy]

Probably not. I'm not a huge fan of allocating the exceptions without throwing them though.

[schveiguy]

I think this can be handled easier by processing the --help option first (possibly outside the Impl function). Then you know while processing each parameter that --help has been passed or not.

[burner]

if you handle help outside impl, the "not so new h|help" functionally would break legacy code, as currently the user can use h and help for anything they want. Changing that is not good IMO. Even if that would re-enable tail-call optimization, if it was present in the first place.

[schveiguy]

This is possible to work around:

1. check to see if any --help option exist in the options array (shouldn't be hard, search for "help|h" string)
2. Do this:

if(helpIncluded)
   return getOptImpl(args, cfg, rslt, options);
else
   return getOptImpl(args, cfg, rslt, "help|h", "This help information.", () {rslt.helpWanted = true;}, options);

And you can avoid all the specific help code in the implementation.

This should be identical to current implementation result, but without triggering exception on --help. As it is, I think the way this is done is terrible. What if the app isn't written in English?

[burner]

the searching part would require to iterate the complete input, properly recursive as it is done currently. I think the cure is worse than the disease (IMO it is not even a problem)

[schveiguy]

the searching part would require to iterate the complete input, properly recursive as it is done currently.

No.

foreach(opt; options)
   static if(is(typeof(opt) == string))
      if(opt == "help|h") helpIncluded = true;

[schveiguy]

Also note, the current code will add a "help" option at the end for displaying help, even if you have handled help on your own.

[burner]

actually I think that is not the case the in-build help is only used if there are no more options to process but you still have arguments left.

[schveiguy]

actually I think that is not the case

import std.getopt;

void main(string[] args)
{
    bool help;
    auto rslt = getopt(args, "help|h", "test help", &help);

    if (help)
    {
        defaultGetoptPrinter("Some information about the program.",
            rslt.options);
    }
}

$ ./testgetopt -h
Some information about the program.
-h --help test help
-h --help This help information.

[burner]

hm, my bad

[schveiguy]

Thinking about this some more. I really am kind of not liking that we handle unspecified required arguments with an exception. Even if we fix this, someone who handles --help on their own will be subject to the same issue.

I don't know a good way to fix this.

[quickfur]

Updated PR title to be more descriptive.

[burner]

@quickfur thanks

[burner]

@schveiguy @quickfur please have a look the tail recursion returned

[quickfur]

LGTM.

[schveiguy]

I'll merge, but I found another problem that will make this problem obsolete :) See https://issues.dlang.org/show_bug.cgi?id=14921

[schveiguy]

Auto-merge toggled on