[m-dev.] for review: deforestation [1/3]

[Fergus Henderson]

Hi,

Sorry about the long delay in reviewing this one.

Simon Taylor <stayl at cs.mu.OZ.AU> wrote:

simplify.m:
> -:- type det_msg_type	--->	warning ; error.
> +:- type det_msg_type	--->	warning ; call_warning ; error.

You're splitting the warnings into two types here,
so I suggest that you name them accordingly: s/warning/simple_code_warning/

> +det_report_msgs_2([Msg | Msgs], Warn, WarnCalls, ModuleInfo,
>  		WarnCnt0, WarnCnt, ErrCnt0, ErrCnt) -->
>  	{ det_msg_get_type(Msg, MsgType) },
>  	( { Warn = no, MsgType = warning } ->
>  		{ WarnCnt1 = WarnCnt0 },
>  		{ ErrCnt1 = ErrCnt0 }
> +	; { WarnCalls = no, MsgType = call_warning } ->
> +		{ WarnCnt1 = WarnCnt0 },
> +		{ ErrCnt1 = ErrCnt0 }
>  	;
>  		det_report_msg(Msg, ModuleInfo),
>  		(

Here as well as s/warning/simple_code_warning/
I also suggest s/Warn/WarnSimple/

>  :- pred det_msg_get_type(det_msg, det_msg_type).
> @@ -905,7 +915,7 @@
>  det_msg_get_type(negated_goal_cannot_succeed(_), warning).
>  det_msg_get_type(warn_obsolete(_, _), warning).
>  det_msg_get_type(warn_infinite_recursion(_), warning).
> -det_msg_get_type(duplicate_call(_, _, _), warning).
> +det_msg_get_type(duplicate_call(_, _, _), call_warning).
>  det_msg_get_type(cc_unify_can_fail(_, _, _, _, _), error).
>  det_msg_get_type(cc_unify_in_wrong_context(_, _, _, _, _), error).
>  det_msg_get_type(cc_pred_in_wrong_context(_, _, _, _), error).

Hmm, I think it is wrong that `warn_obsolete' is controlled by
the setting of the `--warn-simple-code' option.
Of course, that bug was there before your change...

(Notice how using good names for identifiers makes it easier to
spot bugs.  I was going to point out that all these occurrences
of `warning' should be changed to `simple_code_warning', and it
was at that point that I noticed this bug -- in the code
`det_msg_get_type(warn_obsolete(...), simple_code_warning)'
the bug is clearly visible.)

Probably it would be best to add a new alternative, `obsolete_code_warning',
with a new option `--no-warn-obsolete' in options.m.
But this can be a seperate change.  For this change just add an XXX.

> +	% Specify how to process goals - using either
> +	% modes.m or unique_modes.m.
> +:- type how_to_check_goal
> +	--->    check_modes
> +	;       check_unique_modes(may_change_called_proc).
> +
> +	
> +	% Is unique modes allowed to change which procedure of a predicate
> +	% is called. It may not change the called procedure after deforestation
> +	% has performed a generalisation step, since that could result
> +	% in selecting a less efficient mode.
> +:- type may_change_called_proc
> +	--->	may_change_called_proc
> +	;	may_not_change_called_proc.

Please add ", or one which doesn't even have the same termination behaviour"
at the end of the comment here.  (It's not just an efficiency issue, it's
also a correctness issue.)

Otherwise, part 1 looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.