[m-rev.] For review: State Variables

[Simon Taylor]

On 09-May-2002, Ralph Becket <rafe at cs.mu.OZ.AU> wrote:
> Simon Taylor, Monday,  6 May 2002:
> > On 02-May-2002, Ralph Becket <rafe at cs.mu.OZ.AU> wrote:
> > > +		% Handle !.X state variable references.
> > > +		{ F = term__atom("!.") },
> > > +		{ Args = [term__variable(StateVar)] }
> > > +	->
> > > +		dot(Context, StateVar, Var, VarSet0, VarSet,
> > > +			SInfo0, SInfo),
> > > +		{ Goal = svar_unification(Context, X, Var) },
> > > +		{ Info = Info0 }
> > > +	;
> > > +		% Handle !:X state variable references.
> > > +		{ F = term__atom("!:") },
> > > +		{ Args = [term__variable(StateVar)] }
> > > +	->
> > > +		colon(Context, StateVar, Var, VarSet0, VarSet,
> > > +			SInfo0, SInfo),
> > > +		{ Goal = svar_unification(Context, X, Var) },
> > > +		{ Info = Info0 }
> > 
> > The way you've done this the type and mode error messages for
> > state variable references will be really awful. 
> 
> Fixed.

Where are the test cases for the error messages?
 
> > > -get_conj(Goal, Subst, Conj0, VarSet0, Conj, VarSet, Info0, Info) -->
> > > +get_conj(Goal, Subst, Conj0, VarSet0, Conj, VarSet, Info0, Info,
> > > +		SInfo0, SInfo) -->
> > >  	(
> > >  		{ Goal = (A,B) - _Context }
> > >  	->
> > > -		get_conj(B, Subst, Conj0, VarSet0, Conj1, VarSet1,
> > > -						Info0, Info1),
> > > -		get_conj(A, Subst, Conj1, VarSet1, Conj, VarSet, Info1, Info)
> > > +		get_conj(A, Subst, Conj0, VarSet0, Conj1, VarSet1,
> > > +			Info0, Info1, SInfo0, SInfo1),
> > > +		get_conj(B, Subst, Conj1, VarSet1, Conj, VarSet,
> > > +			Info1, Info,  SInfo1, SInfo)
> > >  	;
> > >  		transform_goal(Goal, VarSet0, Subst, Goal1, VarSet,
> > > -						Info0, Info),
> > > +			Info0, Info,  SInfo0, SInfo),
> > >  		{ goal_to_conj_list(Goal1, ConjList) },
> > >  		{ list__append(ConjList, Conj0, Conj) }
> > >  	).
> > 
> > You're now building Conj in reverse, so ConjList needs to be reversed
> > before adding it to Conj0 (same for get_par_conj and get_disj).
> 
> The lists are reversed in the respective callers.

Conj0 and Conj are reversed, ConjList isn't. I'd suggest s/Conj/RevConj/
to avoid confusion (and in get_par_conj and get_disj).
 
> > > +finish_if_then_else(Context, Then0, Then, Else0, Else, VarSet0, VarSet,
> > > +		SInfoT, SInfoE, SInfo) :-
> > > +	SInfo0       = SInfoT,
> > > +	N            = int__max(SInfoT ^ num, SInfoE ^ num),
> > > +	next_svar_info(N, VarSet0, VarSet, SInfo0, SInfo),
> > > +
> > > +	goal_info_init(Context, GoalInfo),
> > > +
> > > +	goal_to_conj_list(Then0, ThenGoals0),
> > > +	ThenUnifiers = svar_unifiers(Context, SInfo ^ dot, SInfoT ^ dot),
> > > +	conj_list_to_goal(ThenGoals0 ++ ThenUnifiers, GoalInfo, Then),
> > > +
> > > +	goal_to_conj_list(Else0, ElseGoals0),
> > > +	ElseUnifiers = svar_unifiers(Context, SInfo ^ dot, SInfoE ^ dot),
> > > +	conj_list_to_goal(ElseGoals0 ++ ElseUnifiers, GoalInfo, Else).
> > 
> > This doesn't handle code like the following properly:
> > 
> > 	( p(!X) ->
> > 		q
> > 	;
> > 		r(!X)
> > 	).
> 
> Fixed.

I'm not totally convinced. You should document somewhere
how this case is handled. There should also be a test case.
 
> --- make_hlds.m	2 May 2002 03:54:54 -0000
> +++ make_hlds.m	9 May 2002 08:26:46 -0000
> @@ -3991,31 +4007,43 @@
>  		ModuleInfo - QualInfo - ClausesInfo) -->
>  	(
>  		{ InstanceClause = clause(CVarSet, PredOrFunc, PredName,
> -				HeadTerms, Body) }
> +				HeadTerms0, Body) }

You should add a test for state variables in instance clauses.

> +:- pred report_svar_unify_error(prog_context, prog_varset, svar, io, io).
> +:- mode report_svar_unify_error(in, in, in, di, uo) is det.
> +
> +report_svar_unify_error(Context, VarSet, StateVar) -->
> +		{ Name = varset__lookup_name(VarSet, StateVar) },
> +		prog_out__write_context(Context),
> +		report_warning(string__format("\
> +Error: !%s cannot appear as a unification argument.\n", [s(Name)])),
> +		globals__io_lookup_bool_option(verbose_errors, VerboseErrors),
> +		(
> +			{ VerboseErrors = yes },
> +			prog_out__write_context(Context),
> +			report_warning(string__format("\
> +       You probably meant !.%s or !:%s.\n", [s(Name), s(Name)]))
> +		;
> +			{ VerboseErrors = no }
> +		).

The `--verbose-errors' part is short enough that you could just
always include it in the message.

> @@ -7614,7 +7689,7 @@
>  :- mode build_lambda_expression(in, in, in, in, in, in, in, in,
>  		in, in, in, out, out, in, out, in, di, uo) is det.
>  
> -build_lambda_expression(X, PredOrFunc, EvalMethod, Args, Modes, Det,
> +build_lambda_expression(X, PredOrFunc, EvalMethod, Args0, Modes, Det,
>  		ParsedGoal, VarSet0, Context, MainContext, SubContext,
>  		Goal, VarSet, Info1, Info, SInfo0) -->
>  	%
> @@ -7652,56 +7727,75 @@
>  	% corresponding to the function result term is a new variable,
>  	% to avoid the function result term becoming lambda-quantified.
>  	%
> -	{ list__length(Args, NumArgs) },
> -	{ varset__new_vars(VarSet0, NumArgs, LambdaVars, VarSet1) },
> -	{ map__init(Substitution) },
> -	{ prepare_for_head(SInfo0, SInfo1) },
> -	{ hlds_goal__true_goal(Head0) },
> -	{ ArgContext = head(PredOrFunc, NumArgs) },
> +	(
> +		{ illegal_state_var_func_result(PredOrFunc, Args0, StateVar) }
> +	->
> +		report_illegal_func_svar_result(Context, VarSet0, StateVar),
> +		{ true_goal(Goal) },
> +		{ VarSet = VarSet0 },
> +		{ Info   = Info1 }
> +	;
> +		{ prepare_for_lambda(SInfo0, SInfo1) },
> +		{ Args1  = expand_bang_state_var_args(Args0) },

That's not right. The following example should be an error.
	(pred(!X::in, !:X::out) is det :- ...) 

> +:- func reconciled_disj_svar_info(prog_varset, hlds_goal_svar_infos) =
> +		svar_info.

This whole section needs more documentation.

> +:- pred substitute_state_var_mappings(list(prog_term), list(prog_term),
> +		prog_varset, prog_varset, svar_info, svar_info, io, io).
> +:- mode substitute_state_var_mappings(in, out, in, out, in, out, di, uo) is det.

Documentation.

> +:- pred report_illegal_func_svar_result(prog_context, prog_varset, svar,
> +		io, io).
> +:- mode report_illegal_func_svar_result(in, in, in, di, uo) is det.
> +
> +report_illegal_func_svar_result(Context, VarSet, StateVar) -->
> +	{ Name = varset__lookup_name(VarSet, StateVar) },
> +	prog_out__write_context(Context),
> +	report_warning(string__format("\
> +Warning: !%s cannot be a function result.\n", [s(Name)])),
> +	globals__io_lookup_bool_option(verbose_errors, Verbose),
> +	(
> +		{ Verbose = yes },
> +		prog_out__write_context(Context),
> +		report_warning(string__format("\
> +         You probably meant !.%s or !:%s.\n", [s(Name), s(Name)]))
> +	;
> +		{ Verbose = no }

The `--verbose-errors' part can just go in with the rest.

Simon.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------