[m-rev.] for review: split getopt*.m's work into three phases

[Zoltan Somogyi]

On Sat, 8 Jan 2022 17:18:21 +1100, Peter Wang <novalazy at gmail.com> wrote:

> On Thu, 06 Jan 2022 16:51:16 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> > 
> > The new code in getopt_io.m uses cords to represent option values,
> > because using reversed lists would be too complex in this application
> > due to the fact they would have to mix with *unreversed* lists.
> > On the other hand, we want to encode the invariant that expanding out
> > file_special options leaves no file_special options in the option value list.
> > This diff does that using insts, which requires cord operations that preserve
> > insts. One consequence of this is that this diff exports the implementation
> > type of cords, though not in the documented part of cord.m's interface.
> > This is undesirable.
> > 
> > I see two possible alternatives. First, we getopt*.m need only a very small
> > part of cord.m's functionality, so we could bodily duplicate that part
> > in getopt*.m, leaving cord.m unchanged. Second, we could use a subtype
> > to record the absence of file_specials, instead of an inst. The latter would
> > require a decision that subtypes are mature enough that we are willing
> > to let the whole Mercury implementation break if they break. Opinions,
> > especially from Peter on the last heading?
> 
> If you don't want to use subtypes yet, there's no *need* to encode the
> invariant, as far as I can see. Just abort if process_options_values
> sees a file_specials (which should be never). That's cleaner than
> secretly exporting some stuff in cord.m.

I neither want to use subtypes nor want to avoid using them; I simply
don't know whether using them represents a risk or not. I presume you
have used them, which is why I asked for your opinion.

If you think they are ready for prime time, I will use them. Otherwise,
I will move the few needed cord ops to getopt*.m. I want the invariant
enforced, not for process_options_values itself, but for the code
that I expect to write in the compiler directory to handle option value lists.
 
> > 
> > There will need to be entries in NEWS, but I want to hold off on that
> > until we agree about yet another issue. The new predicates exported
> > by getopt*.m have an argument that returns an error indication
> > if an error was in fact seen, BUT, even if an error *was* seen, they
> > do return other output as well, such as the list of option values
> > seen before the error. Should this be the case?
> 
> I previously said "yes" (without any justification) so I'll say "yes"
> again. I don't have any use case in mind.

OK.

> > Should we instead return
> > the non-error output *only* in the absence of an error? Having the
> > non-error output up to the error may be useful to some callers,
> > but do we want to commit to preserving the exact same non-error
> > output in the presence of errors? This diff itself affects error behavior,
> > in that before this diff, if an argument list had both a reference to a
> > nonexistent option and a reference to an existing file_special option
> > that named a nonexistent file, we used to report whichever came first
> > on the command line, while after this diff, we will always report the
> > nonexistent option.
> 
> Returning the first error does seem slightly more obvious but it's
> unlikely anyone would ever notice the difference, or care.

That was my reasoning as well.

> > diff --git a/library/getopt_io.m b/library/getopt_io.m
> > index bfefd70f2..c0da9a9bf 100644
> > --- a/library/getopt_io.m
> > +++ b/library/getopt_io.m
> > @@ -126,6 +126,22 @@
> >  
> >  %---------------------------------------------------------------------------%
> >  
> > +:- type short_option(OptionType) == (pred(char, OptionType)).
> > +:- inst short_option ==             (pred(in, out) is semidet).
> > +
> > +:- type long_option(OptionType) ==  (pred(string, OptionType)).
> > +:- inst long_option ==              (pred(in, out) is semidet).
> > +
> > +:- type option_defaul_value(OptionType) == (pred(OptionType, option_data)).
> > +:- inst option_defaul_value_nondet ==      (pred(out, out) is nondet).
> > +:- inst option_defaul_value_multi ==       (pred(out, out) is multi).
> 
> default

Will fix, both this and the spaces.

Thanks for the review.

Zoltan.