[m-rev.] for review: state vars syntax sugar
Ralph Becket
rafe at csse.unimelb.edu.au
Thu Jan 25 07:17:52 AEDT 2007
Hi Ondrej,
thanks for doing this. Review comments below:
Ondrej Bojar, Wednesday, 24 January 2007:
> For review by anyone (Ralph?, Julien?).
>
> This is the proposed syntactic sugar on state variables in pred/mode
> declarations and lambda expressions. A testcase is at the end.
>
> Hours taken: 5, but this includes orientation in the compiler code and
> development of a simple 'ctags/tags' generator for Mercury.
>
> Here is the diff, bootcheck is now running, the test case seems to compile.
>
> Ondrej.
Three things first:
(1) You need to add an entry to the Reference Manual.
(2) You need to add an entry to the NEWS file.
(3) For consistency in the concrete syntax, I think a state-variable
mode pair should also be preceeded by !. That is, the following should
be equivalent:
:- pred foo(!bar::!(in, out)) is det.
:- pred foo(!bar).
:- mode foo(!(in, out)) is det.
:- pred foo(bar, bar).
:- mode foo(in, out) is det.
The ! symbol immediately alerts the reader to the fact that state
variables are being used. Just using a (_, _) pair for the modes
omits this helpful visual clue.
>
>
> Index: compiler/prog_data.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/prog_data.m,v
> retrieving revision 1.187
> diff -u -r1.187 prog_data.m
> --- compiler/prog_data.m 15 Jan 2007 10:30:35 -0000 1.187
> +++ compiler/prog_data.m 24 Jan 2007 06:42:37 -0000
> @@ -59,6 +59,9 @@
> ; promise_type_true.
> % Promise goal is true.
>
> + % Generic type to return one or two values of a type
> +:- type one_or_two(T) ---> one(T); two(T, T).
> +
Our coding format in the compiler is to write this as
:- type one_or_two(T)
---> one(T)
; two(T).
But for code maintenance purposes you would do better to rename this
type and its constructors:
:- type normal_arg_or_state_var_pair(T)
---> normal_arg(T)
; state_var_pair(T, T).
Also prog_io_util.m is probably a better home for this type.
> :- type type_and_mode
> ---> type_only(mer_type)
> ; type_and_mode(mer_type, mer_mode).
> Index: compiler/prog_io.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/prog_io.m,v
> retrieving revision 1.281
> diff -u -r1.281 prog_io.m
> --- compiler/prog_io.m 19 Jan 2007 07:04:27 -0000 1.281
> +++ compiler/prog_io.m 24 Jan 2007 06:41:47 -0000
> @@ -3133,38 +3133,48 @@
> Msg = "some but not all arguments have modes",
> Result = error1([Msg - FuncTerm])
> ;
> - convert_type_and_mode(InstConstraints, ReturnTypeTerm,
> ReturnType)
> + convert_type_and_mode(InstConstraints, ReturnTypeTerm,
> + ReturnTypeOrTwo)
The continuation indentation here should be to the next tab (i.e., add
another two spaces before ReturnTypeOrTwo).
> ->
> (
> - As = [type_and_mode(_, _) | _],
> - ReturnType = type_only(_)
> - ->
> - Msg = "function arguments have modes, " ++
> - "but function result doesn't",
> - Result = error1([Msg - FuncTerm])
> - ;
> - As = [type_only(_) | _],
> - ReturnType = type_and_mode(_, _)
> - ->
> - Msg = "function result has mode, but function arguments
> don't",
> - Result = error1([Msg - FuncTerm])
> - ;
> - get_purity(Purity, Attributes0, Attributes),
> - varset.coerce(VarSet0, TVarSet),
> - varset.coerce(VarSet0, IVarSet),
> - list.append(As, [ReturnType], Args),
> + ReturnTypeOrTwo = two(_, _),
> + Msg = "function result cannot be a state variable",
> + Result = error1([Msg-FuncTerm])
> + ; ReturnTypeOrTwo = one(ReturnType),
> (
That should be formatted as
ReturnTypeOrTwo = two(_, _),
Msg = "function result cannot be a state variable",
Result = error1([Msg-FuncTerm])
;
ReturnTypeOrTwo = one(ReturnType),
(
> -
> inst_var_constraints_are_consistent_in_type_and_modes(Args)
> + As = [type_and_mode(_, _) | _],
> + ReturnType = type_only(_)
> ->
> - Origin = user,
> - Result0 = ok1(item_pred_or_func(Origin, TVarSet,
> IVarSet,
> - ExistQVars, pf_function, F, Args, no, no, MaybeDet,
> - Cond, Purity, ClassContext)),
> - check_no_attributes(Result0, Attributes, Result)
> + Msg = "function arguments have modes, " ++
> + "but function result doesn't",
> + Result = error1([Msg - FuncTerm])
> ;
> - Msg = "inconsistent constraints on inst variables " ++
> - "in function declaration",
> - Result = error1([Msg - FullTerm])
> + As = [type_only(_) | _],
> + ReturnType = type_and_mode(_, _)
> + ->
> + Msg = "function result has mode, " ++
> + "but function arguments don't",
> + Result = error1([Msg - FuncTerm])
> + ;
> + get_purity(Purity, Attributes0, Attributes),
> + varset.coerce(VarSet0, TVarSet),
> + varset.coerce(VarSet0, IVarSet),
> + list.append(As, [ReturnType], Args),
> + (
> +
> inst_var_constraints_are_consistent_in_type_and_modes(
> + Args)
> + ->
> + Origin = user,
> + Result0 = ok1(item_pred_or_func(Origin, TVarSet,
> + IVarSet,
> + ExistQVars, pf_function, F, Args, no, no,
> MaybeDet,
> + Cond, Purity, ClassContext)),
> + check_no_attributes(Result0, Attributes, Result)
> + ;
> + Msg = "inconsistent constraints on inst
> variables " ++
> + "in function declaration",
> + Result = error1([Msg - FullTerm])
> + )
> )
> )
> ;
> @@ -3690,22 +3700,60 @@
> list(type_and_mode)::out) is semidet.
>
> convert_type_and_mode_list(_, [], []).
> -convert_type_and_mode_list(InstConstraints, [H0 | T0], [H | T]) :-
> - convert_type_and_mode(InstConstraints, H0, H),
> - convert_type_and_mode_list(InstConstraints, T0, T).
> +convert_type_and_mode_list(InstConstraints, [H0 | T0], Out) :-
> + convert_type_and_mode(InstConstraints, H0, OneOrTwoH),
> + convert_type_and_mode_list(InstConstraints, T0, T),
> + (
> + OneOrTwoH = one(H),
> + Out = [H | T]
> + ;
> + OneOrTwoH = two(H1, H2),
> + Out = [H1, H2 | T]
> + ).
That should be formatted as
convert_type_and_mode_list(InstConstraints, [H0 | T0], Out) :-
convert_type_and_mode(InstConstraints, H0, OneOrTwoH),
convert_type_and_mode_list(InstConstraints, T0, T),
(
OneOrTwoH = one(H),
Out = [H | T]
;
OneOrTwoH = two(H1, H2),
Out = [H1, H2 | T]
).
And I'd rename OneOrTwoH as just OneOrTwo.
>
> -:- pred convert_type_and_mode(inst_var_sub::in, term::in,
> type_and_mode::out)
> +:- pred convert_type_and_mode(inst_var_sub::in, term::in,
> one_or_two(type_and_mode)::out)
> is semidet.
>
> +:- pred debugstr(string::in, T::in, io::di, io::uo) is det.
> +debugstr(Msg, Data, !IO) :-
> + io__stderr_stream(E, !IO),
> + io__write_string(E, Msg, !IO),
> + io__write(E, Data, !IO),
> + io__nl(E, !IO).
Remove this predicate (and we don't use `__' as a module qualifier any
more).
> +
> convert_type_and_mode(InstConstraints, Term, Result) :-
> - ( Term = term.functor(term.atom("::"), [TypeTerm, ModeTerm],
> _Context) ->
> - parse_type(TypeTerm, ok1(Type)),
> - convert_mode(allow_constrained_inst_var, ModeTerm, Mode0),
> - constrain_inst_vars_in_mode(InstConstraints, Mode0, Mode),
> - Result = type_and_mode(Type, Mode)
> + ( Term = term.functor(term.atom("::"), [TypeTerm, ModeTerm], _Context)
> + ->
> + % trace[io(!IO)]debugstr("Type: ", TypeTerm, !IO),
> + % trace[io(!IO)]debugstr("Mode: ", ModeTerm, !IO),
Delete the trace goals (see below).
> + ( (
> + TypeTerm = term.functor(term.atom("!"), [TypeTerm2], _Context2),
> + ModeTerm = term.functor(term.atom(","), [InModeTerm,
> OutModeTerm], _)
> + )
> + ->
> + parse_type(TypeTerm2, ok1(Type)),
> + convert_mode(allow_constrained_inst_var, InModeTerm, InMode0),
> + constrain_inst_vars_in_mode(InstConstraints, InMode0, InMode),
> + convert_mode(allow_constrained_inst_var, OutModeTerm, OutMode0),
> + constrain_inst_vars_in_mode(InstConstraints, OutMode0, OutMode),
> + Result = two(type_and_mode(Type, InMode),
> + type_and_mode(Type, OutMode))
> + ;
> + parse_type(TypeTerm, ok1(Type)),
> + convert_mode(allow_constrained_inst_var, ModeTerm, Mode0),
> + constrain_inst_vars_in_mode(InstConstraints, Mode0, Mode),
> + Result = one(type_and_mode(Type, Mode))
> + ),
> + % trace[io(!IO)]debugstr("Result: ", Result, !IO),
> + true
Delete the trace goal and extraneous true.
> + ;
> + Term = term.functor(term.atom("!"), [Term2], _Context)
> + ->
> + parse_type(Term2, ok1(Type)),
> + Result = two(type_only(Type), type_only(Type))
Your formatting here is non-standard; here's the style we use in the
compiler:
(
Term = term.functor(term.atom("::"), [TypeTerm, ModeTerm], _Context)
->
(
TypeTerm =
term.functor(term.atom("!"), [TypeTerm2], _Context2),
ModeTerm =
term.functor(term.atom(","), [InModeTerm, OutModeTerm], _)
->
parse_type(TypeTerm2, ok1(Type)),
convert_mode(allow_constrained_inst_var, InModeTerm, InMode0),
constrain_inst_vars_in_mode(InstConstraints, InMode0, InMode),
convert_mode(allow_constrained_inst_var, OutModeTerm, OutMode0),
constrain_inst_vars_in_mode(InstConstraints, OutMode0, OutMode),
Result =
two(type_and_mode(Type, InMode), type_and_mode(Type, OutMode))
;
parse_type(TypeTerm, ok1(Type)),
convert_mode(allow_constrained_inst_var, ModeTerm, Mode0),
constrain_inst_vars_in_mode(InstConstraints, Mode0, Mode),
Result = one(type_and_mode(Type, Mode))
)
;
Term = term.functor(term.atom("!"), [Term2], _Context)
->
parse_type(Term2, ok1(Type)),
Result = two(type_only(Type), type_only(Type))
> ;
> parse_type(Term, ok1(Type)),
> - Result = type_only(Type)
> + Result = one(type_only(Type))
> ).
>
> :- pred make_mode_defn(varset::in, condition::in, processed_mode_body::in,
> Index: compiler/prog_io_goal.m
> ===================================================================
> RCS file:
> /home/mercury/mercury1/repository/mercury/compiler/prog_io_goal.m,v
> retrieving revision 1.50
> diff -u -r1.50 prog_io_goal.m
> --- compiler/prog_io_goal.m 1 Dec 2006 15:04:16 -0000 1.50
> +++ compiler/prog_io_goal.m 24 Jan 2007 07:01:03 -0000
> @@ -1037,14 +1037,35 @@
>
>
> %-----------------------------------------------------------------------------%
>
> -:- pred parse_lambda_arg(term::in, prog_term::out, mer_mode::out) is
> semidet.
> -
> -parse_lambda_arg(Term, ArgTerm, Mode) :-
> +:- pred parse_non_sv_lambda_arg(term::in, prog_term::out,
> mer_mode::out) is semidet.
> +parse_non_sv_lambda_arg(Term, ArgTerm, Mode) :-
> Term = term.functor(term.atom("::"), [ArgTerm0, ModeTerm], _),
> term.coerce(ArgTerm0, ArgTerm),
> convert_mode(allow_constrained_inst_var, ModeTerm, Mode0),
> constrain_inst_vars_in_mode(Mode0, Mode).
>
> +:- pred parse_lambda_arg(term::in, one_or_two({prog_term,
> mer_mode})::out) is semidet.
> +
> +parse_lambda_arg(Term, Out) :-
> + Term = term.functor(term.atom("::"), [ArgTerm0, ModeTerm], _),
> + ( ArgTerm0 = term.functor(term.atom("!"), [ArgTerm1], Context),
> + ModeTerm = term.functor(term.atom(","), [InModeTerm, OutModeTerm], _)
> + -> % expand state variable into two
> + term.coerce(ArgTerm1, ArgTerm),
> + InArgTerm = term.functor(term.atom("!."), [ArgTerm], Context),
> + OutArgTerm = term.functor(term.atom("!:"), [ArgTerm], Context),
> + convert_mode(allow_constrained_inst_var, InModeTerm, InMode0),
> + constrain_inst_vars_in_mode(InMode0, InMode),
> + convert_mode(allow_constrained_inst_var, OutModeTerm, OutMode0),
> + constrain_inst_vars_in_mode(OutMode0, OutMode),
> + Out = two({InArgTerm, InMode}, {OutArgTerm, OutMode})
> + ;
> + term.coerce(ArgTerm0, ArgTerm),
> + convert_mode(allow_constrained_inst_var, ModeTerm, Mode0),
> + constrain_inst_vars_in_mode(Mode0, Mode),
> + Out = one({ArgTerm, Mode})
> + ).
> +
Same problem with formatting here.
> %-----------------------------------------------------------------------------%
> %
> % Code for parsing pred/func expressions
> @@ -1075,7 +1096,7 @@
> FuncArgsTerm = term.functor(term.atom("func"), FuncArgsList, _),
>
> ( parse_pred_expr_args(FuncArgsList, Args0, Modes0) ->
> - parse_lambda_arg(RetTerm, RetArg, RetMode),
> + parse_non_sv_lambda_arg(RetTerm, RetArg, RetMode),
> list.append(Args0, [RetArg], Args),
> list.append(Modes0, [RetMode], Modes),
> inst_var_constraints_are_consistent_in_modes(Modes)
> @@ -1114,9 +1135,19 @@
> list(mer_mode)::out) is semidet.
>
> parse_pred_expr_args([], [], []).
> -parse_pred_expr_args([Term | Terms], [Arg | Args], [Mode | Modes]) :-
> - parse_lambda_arg(Term, Arg, Mode),
> - parse_pred_expr_args(Terms, Args, Modes).
> +parse_pred_expr_args([Term | Terms], ResultArgs, ResultModes) :-
> + parse_lambda_arg(Term, OneOrTwo),
> + parse_pred_expr_args(Terms, Args, Modes),
> + (
> + OneOrTwo = one({Arg, Mode}),
> + ResultArgs = [Arg | Args],
> + ResultModes = [Mode | Modes]
> + ;
> + OneOrTwo = two({InArg, InMode}, {OutArg, OutMode}),
> + ResultArgs = [InArg, OutArg | Args],
> + ResultModes = [InMode, OutMode | Modes]
> + ).
> +
Same problem with formatting here.
>
> % parse_dcg_pred_expr_args is like parse_pred_expr_args except
> % that the last two elements of the list are the modes of the
> @@ -1131,9 +1162,18 @@
> convert_mode(allow_constrained_inst_var, DCGModeTermB, DCGModeB0),
> constrain_inst_vars_in_mode(DCGModeA0, DCGModeA),
> constrain_inst_vars_in_mode(DCGModeB0, DCGModeB).
> -parse_dcg_pred_expr_args([Term | Terms], [Arg | Args], [Mode | Modes]) :-
> +parse_dcg_pred_expr_args([Term | Terms], ResultArgs, ResultModes) :-
> Terms = [_, _ | _],
> - parse_lambda_arg(Term, Arg, Mode),
> - parse_dcg_pred_expr_args(Terms, Args, Modes).
> + parse_lambda_arg(Term, OneOrTwo),
> + parse_dcg_pred_expr_args(Terms, Args, Modes),
> + (
> + OneOrTwo = one({Arg, Mode}),
> + ResultArgs = [Arg | Args],
> + ResultModes = [Mode | Modes]
> + ;
> + OneOrTwo = two({InArg, InMode}, {OutArg, OutMode}),
> + ResultArgs = [InArg, OutArg | Args],
> + ResultModes = [InMode, OutMode | Modes]
> + ).
Same problem with formatting here.
>
>
> %-----------------------------------------------------------------------------%
> Index: compiler/prog_io_util.m
> ===================================================================
> RCS file:
> /home/mercury/mercury1/repository/mercury/compiler/prog_io_util.m,v
> retrieving revision 1.57
> diff -u -r1.57 prog_io_util.m
> --- compiler/prog_io_util.m 19 Jan 2007 07:04:28 -0000 1.57
> +++ compiler/prog_io_util.m 24 Jan 2007 06:03:09 -0000
> @@ -456,9 +456,20 @@
> Term = term.functor(term.atom("impure"), [Term0], Context).
>
> convert_mode_list(_, [], []).
> -convert_mode_list(AllowConstrainedInstVar, [H0 | T0], [H | T]) :-
> - convert_mode(AllowConstrainedInstVar, H0, H),
> - convert_mode_list(AllowConstrainedInstVar, T0, T).
> +convert_mode_list(AllowConstrainedInstVar, [H0 | T0], Out) :-
> + (
> + H0 = term.functor(term.atom(","), [InModeTerm, OutModeTerm], _)
> + ->
> + % a pair of modes in () used for state variables
> + convert_mode(AllowConstrainedInstVar, InModeTerm, InMode),
> + convert_mode(AllowConstrainedInstVar, OutModeTerm, OutMode),
> + Prefix = [InMode, OutMode]
> + ;
> + convert_mode(AllowConstrainedInstVar, H0, H),
> + Prefix = [H]
> + ),
> + convert_mode_list(AllowConstrainedInstVar, T0, T),
> + Out = Prefix ++ T.
Same problem with formatting here.
You might need to module qualify that ++ as
Out = list.(Prefix ++ T).
Also, H and T are not really good names for parts of a list when you
know it's a list of modes (I appreciate this problem was already present
in the compiler code, but since you're making the change...!)
>
> convert_mode(AllowConstrainedInstVar, Term, Mode) :-
> (
> Index: tests/valid/state_var_mode.m
> ===================================================================
> RCS file: tests/valid/state_var_mode.m
> diff -N tests/valid/state_var_mode.m
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ tests/valid/state_var_mode.m 24 Jan 2007 07:11:29 -0000
> @@ -0,0 +1,31 @@
> +% this is a sample of syntactic sugar for predicate mode declarations
> +% for state variables
> +:- module state_var_mode.
> +:- interface.
> +
> +:- type foo ---> foo; bar.
> +
> +:- pred p1(!foo::(in,out)) is det.
> +
> +
> +:- pred p2(!foo).
> +% :- mode p2(in, out) is det. % old syntax
> +:- mode p2((in, out)) is det.
> +
> +:- func f1(!foo::(in,out)) = (foo::out) is det.
> +
> +:- implementation.
> +:- import_module list.
> +
> +p1(!IO) :-
> + list__map(
> + % (pred(!.I::in, !:I::out) is det :- % old syntax
> + (pred(!I::(in, out)) is det :-
> + (!.I = foo, !:I = bar
> + ;!.I = bar, !:I = foo)
> + ), [foo, bar], _).
> +p2(!I) :-
> + (!.I = foo, !:I = bar
> + ;!.I = bar, !:I = foo).
> +
> +f1(!I) = foo.
Formatting problems. You need to try all three combinations from my
point (3) at the start of this review comment.
Thanks again for making the change!
-- Ralph
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------
More information about the reviews
mailing list