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

[Peter Wang]

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.

> 
> 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.

> 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.

> 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

> +
> +:- type special_handler(OptionType) == 
> +    (pred(OptionType, special_data,
> +        option_table(OptionType), maybe_option_table(OptionType))).
> +:- inst special_handler == 
> +    (pred(in, in, in, out) is semidet).
> +

Delete trailing whitespace.

> +%---------------------------------------------------------------------------%
> +
> +    % An inst that lists all the function symbols of the option_value type
> +    % *except* ov_file_special.
> +:- inst non_file_special for option_value/1
> +    --->    ov_bool(ground, ground, ground)
> +    ;       ov_int(ground, ground, ground)
> +    ;       ov_string(ground, ground, ground)
> +    ;       ov_maybe_int(ground, ground, ground)
> +    ;       ov_maybe_string(ground, ground, ground)
> +    ;       ov_accumulating(ground, ground, ground)
> +    ;       ov_accumulating_reset(ground, ground)
> +    ;       ov_special(ground, ground)
> +    ;       ov_bool_special(ground, ground, ground)
> +    ;       ov_int_special(ground, ground, ground)
> +    ;       ov_string_special(ground, ground, ground)
> +    ;       ov_maybe_string_special(ground, ground, ground).
> +
> +    % expand_file_specials(OptionOps, OptionTable, OptionValues, 
> +    %   MaybeError, NonFileSpecialOptionValues, !MaybeIO):

Delete trailing space.

The rest looks okay.

Peter