[m-rev.] for review: library grade set filters

[Julien Fischer]

On Tue, 6 Nov 2007, Peter Wang wrote:

> On 2007-11-05, Julien Fischer <juliensf at csse.unimelb.edu.au> wrote:

>>  +
>>  +    % string_to_grade_component(OptionStr, Comp, !Comps, !Errors):
>>  +    %
>>  +    % If `Comp' is a string that represents a valid grade component
>>  +    % then add it to !Comps.  If it is not then emit an error message.
>>  +    % `OptionStr' should be the name of the command line option for
>>  +    % which the error is to be reported.
>>  +    %
>>  +:- pred string_to_grade_component(string::in, string::in,
>>  +    list(string)::in, list(string)::out,
>>  +    list(string)::in, list(string)::out) is det.
>>  +
>>  +string_to_grade_component(OptionStr, Comp, !Comps, !Errors) :-
>>  +    ( grade_component_table(Comp, _, _, _, _) ->
>>  +        !:Comps = [Comp | !.Comps]
>>  +    ;
>>  +        add_error("`" ++ OptionStr ++ "' unknown grade component:  "
>>  +            ++ Comp, !Errors)
>>  +    ).
>
> Wouldn't that introduce a (slight) double maintenance problem, since the
> option names are listed here and in options.m?

Yes, but it's only very slight.

> And the user might have
> written `--libgrades-include' instead of `--libgrades-include-component'.

`--libgrades-include' is documented as being a synonym for the latter.

> So maybe it's better to leave out the option string from the error
> message.

I think it's better to give the user at least some idea of where the
error is, e.g. in the following

 	mmm --grade asm_fast.gc.tr.profdeep.stseg \
 		--libgrades-exclude-component spf \
 		--libgrades-exclude-component reg \
 		--libgrades-include-component tr  \
 		--libgrades-include-component gc

I think, at the least, it would be good to know which flag has an
incorrect argument.  How about this?

 	unknown included library grade component: <component>

and likewise substituting excluded for included.

>>  +
>>  +    % filter_grade(FilterPred, Components, GradeString, !Grades, !Errors):
>>  +    % +    % Convert `GradeString' into a list of grade component strings,
>>  and
>>  +    % then check whether the given grade should be filtered from the
>>  +    % library grade set by applying the closure `FilterPred(Components)',
>>  +    % to that list.  The grade removed from the library grade set if
>>  +    % the application fails.
>
> is removed ... if that application fails.

Fixed.

>>  +:- pred must_contain(list(string)::in, list(string)::in) is semidet.
>>  +
>>  +must_contain(IncludeComponents, GradeComponents) :-
>>  +    all [Component] ( list.member(Component, IncludeComponents) =>
>>  +        list.member(Component, GradeComponents)
>>  +    ).
>>  +
>>  +:- pred must_not_contain(list(string)::in, list(string)::in) is semidet.
>>  +
>>  +must_not_contain(OmitComponents, GradeComponents) :-
>>  +    all [Component] (list.member(Component, OmitComponents) =>
>>  +        not list.member(Component, GradeComponents)
>>  +    ).
>>  +
>
> I prefer
>
> 	all [X] (
> 	    p(X)
> 	=>
> 	    ...
> 	)

Okay.

>>  Index: compiler/options.m
>>  ===================================================================
>>  RCS file: /home/mercury/mercury1/repository/mercury/compiler/options.m,v
>>  retrieving revision 1.598
>>  diff -u -r1.598 options.m
>>  --- compiler/options.m	31 Oct 2007 03:58:28 -0000	1.598
>>  +++ compiler/options.m	5 Nov 2007 06:16:27 -0000
> ...
>>  @@ -4989,6 +4997,18 @@
>>           "--no-libgrade",
>>           "\tClear the list of compilation grades in which a library",
>>           "\tto be installed should be built.",
>>  +        "--libgrades-include-component <component>",
>>  +        "--libgrades-include <component>",
>>  +        "\tFilter the list of compilation grades in which a library to",
>>  +        "\tbe installed should be built so that it only contains grades",
>>  +        "\tthat include the specified grade component.",
>>  +        "\t(This option does not work with Mmake, only `--make'.)",
>
> It's a bit hard to read.
>
>    In the list of grades in which to install a library, keep only the
>    grades which include the specified grade component.

How about:

 	Remove those grades that do not contain the specified component
 	from the set of library grades to be installed.


>>  +        "--libgrades-omit-component <component>",
>>  +        "--libgrades-omit <component>",
>>  +        "\tFilter the list of compilation grades in which a library to",
>>  +        "\tbe installed should be built so that it does not contain any",
>>  +        "\tgrades that include the specified grade component.",
>>  +        "\t(This option does not work with Mmake, only `--make'.)",
>
>    In the list of grades in which to install a library, remove any
>    grade which includes the specified grade component.
>
>>  Index: doc/user_guide.texi
>>  ===================================================================
>>  RCS file: /home/mercury/mercury1/repository/mercury/doc/user_guide.texi,v
>>  retrieving revision 1.549
>>  diff -u -r1.549 user_guide.texi
>>  --- doc/user_guide.texi	25 Oct 2007 06:53:44 -0000	1.549
>>  +++ doc/user_guide.texi	5 Nov 2007 06:16:27 -0000
>>  @@ -7170,7 +7170,7 @@
>>   @samp{spf} (the default is to use double-precision floats)
>>
>>   @item Whether to allocate a new stack segment when the limit on the old one
>>  -is reached: @samp{stseg} (the default is not to allocate a new stack
>>  segement)
>>  +is reached: @samp{stseg} (the default is not to allocate a new stack
>>  segment)
>
> The description is correct but doesn't explain the reasoning.  I
> suggest:
>
>    @item Whether to use dynamically-sized stacks, composed of small segments:
>    @samp{stseg} (the default is to use fixed-sized stacks)
>
> with a paragraph to be added later to `LLDS back-end compilation model
> options' explaining that it increases safety and (almost certainly)
> reduces memory consumption, at the expense of speed.

I'll deal with this separately; it's not really part of this change.

>>   @item What debugging features to enable:
>>   @samp{debug} and @samp{decldebug} (the default is no debugging features).
>>  @@ -8731,6 +8731,26 @@
>>   building and installing the default set of grades.
>>
>>   @sp 1
>>  + at item --libgrades-include-component @var{component}
>>  + at itemx --libgrades-include @var{component}
>>  + at findex --libgrade-include-component
>>  + at findex --libgrades-include
>>  +Filter the list of compilation grades in which a library to be
>>  +installed should be built so that it only contains grades
>>  +that include the specified grade component.
>>  +(This option does not work with Mmake, only @samp{--make}.)
>
> only @samp{mmc --make}

Fixed.

>>  +
>>  + at sp 1
>>  + at item --libgrades-omit-component @var{component}
>>  + at itemx --libgrade-omit @var{component}
>>  + at findex --libgrades-omit-component
>>  + at findex --libgrade-omit
>>  +Filter the list of compilation grades in which a library to be
>>  +installed should be built so that it does not contain any grades
>>  +that include the specified grade component.
>>  +(This option does not work with Mmake, only @samp{--make}.)
>
> only @samp{mmc --make}

Fixed.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------