docopt.rs icon indicating copy to clipboard operation
docopt.rs copied to clipboard

Remove seemingly unnecessary Repeat patterns

Open bvinc opened this issue 7 years ago • 1 comments

This should make the tests in PR #185 much faster.

This was the pattern before my change:

r"""Usage:
  prog a b [-p <param>]...
  prog a c [-p <type> <param>]...
"""
$ program a c -p bool 1 -p bool 0 -p bool 1
{"-p": 3, "<type>": ["bool", "bool", "bool"], "<param>": ["1", "0", "1"]}

Usages:
  Sequence([PatAtom(Command("a")), PatAtom(Command("b")), Repeat(Optional([PatAtom(Short('p')), PatAtom(Positional("param"))]))])
  Sequence([PatAtom(Command("a")), PatAtom(Command("c")), Repeat(Optional([Repeat(PatAtom(Short('p'))), PatAtom(Positional("type")), PatAtom(Positional("param"))]))])

The Repeat(Optional([Repeat(... causes an explosion in the function states. states was being called 1,421,237 times, and producing a vector of 236,975 elements.

This is the pattern after:

Usages:
  Sequence([PatAtom(Command("a")), PatAtom(Command("b")), Repeat(Optional([PatAtom(Short('p')), PatAtom(Positional("param"))]))])
  Sequence([PatAtom(Command("a")), PatAtom(Command("c")), Repeat(Optional([PatAtom(Short('p')), PatAtom(Positional("type")), PatAtom(Positional("param"))]))])

states is being called 7,843 times, and producing a vector of 2,296 elements.

I'm not sure why the removed code was ever written. The concept of "has_repeat" means that the short can possibly repeat under some context somewhere, which seems to have a completely separate meaning from the Repeat(...) pattern. Furthermore, this extra Repeat(...) pattern only affects the second sequence and not the first. That's because after the first sequence is processed, Short('p') is flagged as something that repeats, which affects only the second sequence.

bvinc avatar Oct 04 '16 05:10 bvinc

Hmm, that code was added in a4ee9b to support this feature: https://github.com/docopt/docopt/issues/275

I find it curious that the tests still pass, but I can't yet explain why I added that code in particular. I'll take a closer look when I get a chance.

Thank you for looking into this. :-)

BurntSushi avatar Oct 04 '16 10:10 BurntSushi