[m-rev.] for review: generalize the deconstruction predicates

[Fergus Henderson]

On 31-Jan-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> For review by anyone.
> 
> Reorganize deconstruct.m so that each predicate that deconstructs terms has
> three variants:
> 
> - One that aborts when attempting to deconstruct non-canonical terms.

It would be nicer to throw an exception rather than aborting.

There should at least be an XXX comment in the appropriate place
in the implementation.

> library/store.m:
> 	Conform to the new interfaces of some functions in the updated files in
> 	the runtime.

You need to modify extras/trailed_update/tr_store.m likewise.

> library/Mmakefile:
> 	Suppress some warnings to work around bug #2.
...
> +# XXX The following are needed because a bug in purity checking, which gives
> +# an error if a promise_pure declaration in deconstruct.m is omitted and a
> +# warning about the declaration being unnecessary in deconstruct.opt
> +# if it is included.
>  #
>  
> +MCFLAGS-deconstruct = --no-halt-at-warn
> +MCFLAGS-std_util = --no-halt-at-warn

Why do you need MCFLAGS-deconstruct = --no-halt-at-warn?
Doesn't the warning only occur when compiling other modules
that import deconstruct, not when compiling deconstruct itself?
When compiling deconstruct itself, the compiler shouldn't
read deconstruct.opt.

> +++ library/deconstruct.m	31 Jan 2002 08:23:06 -0000
> @@ -17,6 +17,26 @@
>  
>  :- import_module std_util, list.
>  
> +:- type noncanon_handling
> +	--->	do_not_allow	% Abort if an operation tries to deconstruct
> +				% a noncanonical value.

There should be a brief comment at the start explaining the purpose of
the type as a whole, e.g. "Specify how to handle attempts to deconstruct
values of noncanonical types", along with a description of (or pointer to
a description of) what "noncanonical types" are.

> +	;	canonicalize	% If an operation tries to deconstruct a value
> +				% of a noncanonical type, make it return a
> +				% marker that says so. This marker will be the
> +				% same for all values of that noncanonical
> +				% type. Operations will still abort if they try
> +				% to deconstruct a variable (this should only
> +				% affect HAL users).

I don't think the last sentence should be included in the library
reference manual.

At most, it should be entirely parenthetical:

	(Note for HAL implementors: ...)

But I think it would be best to just delete it from here,
and put them comment in the implementation section.

Or alternatively, it could be included in the source, but tagged in some
way that ensures that it doesn't go into the library reference manual.

Note that the only way to notice that `deconstruct' aborts when passed
a variable already involves lying to the compiler (passing an unbound
variable to something which has mode `in' rather than `in(any)').
If you lie to the compiler, and it aborts with a reasonable error message,
then you're probably already getting a better deal than you deserve ;-)
We don't need to document here that lying to the compiler may result
in runtime aborts.

> +:- inst do_not_allow == bound(do_not_allow).

I suggest using the syntax

	:- inst do_not_allow ---> do_not_allow.

> @@ -27,6 +47,8 @@
>  	% 	  in the type definition. For lists, this
>  	% 	  means the functors [|]/2 and []/0 are used, even if
>  	% 	  the list uses the [....] shorthand.
> +	%	  For types with user-defined equality, the functor will be
> +	%	  <<noncanonical>>/0 except with include_details_cc.

IMHO it would be better to make the functor be the name of the type,
e.g. '<<var:var/1>>'/0.

In fact the documentation here doesn't match the code --
the code returns the functor "noncanonical", rather than
"<<noncanonical>>".

>  	%	- for integers, the string is a base 10 number,
>  	%	  positive integers have no sign.
>  	%	- for floats, the string is a floating point,
> @@ -38,6 +60,8 @@
>  	%	- for functions, the string <<function>>
>  	%	- for tuples, the string {}
>  	%	- for arrays, the string <<array>>
> +	%	- for type_infos, the string <<typeinfo>>
> +	%	- for type_ctor_infos, the string <<typectorinfo>>

What are type_infos and type_ctor_infos?  Those are not documented
anywhere in the language or library reference manual.

(Again, this documentation could be included in the source, but only
if tagged in some way that ensures that it doesn't go into the library
reference manual.)

It should instead document here what happens for type_descs and
type_ctor_descs.

The behaviour which I think is desirable for type_descs is for
the functor returned to be the module-qualified name of the type
that the type_desc represents, and for the arity returned
to be that type's arity.

For type_ctor_descs, the name returned should be the module-qualified
name and arity of the type constructor, and the arity should be zero.

(However, changing it to use that behaviour can be a separate change.)

> +:- pred functor(T, noncanon_handling, string, int).
> +:- mode functor(in, in(do_not_allow), out, out) is det.
> +:- mode functor(in, in(canonicalize), out, out) is det.
> +:- mode functor(in, in(include_details_cc), out, out) is cc_multi.

It might also be useful to allow the handling of noncanonical types
to be determined at runtime, i.e. adding a mode

	:- mode functor(in, in, out, out) is cc_multi.

> +% The predicates univ_arg/4 and univ_named_arg/4 are used only to work around
> +% the typechecking bug reported on 30 Jan, 2002.

Hmm... readers may not easily know which bug that was.
Log the bug on SourceForge, and then include the bug URL in the comment.
Likewise for the other places in this diff where you refer to that bug.

Also, the comment should start with an "XXX".

> +% The no-inline pragmas are necessary because when it inlines a predicate
> +% defined by foreign_procs, the compiler does not preserve the names of the
> +% typeinfo variables. Thus these foreign_proc's references to TypeInfo_for_T
> +% will refer to an undefined variable.
> +
> +:- pragma no_inline(functor/4).

The comment here should start with an "XXX".

>  :- pragma foreign_proc("C",
> -	functor(Term::in, Functor::out, Arity::out),
> +	functor(Term::in, NonCanon::in(do_not_allow), Functor::out,
> +		Arity::out),
>  	[will_not_call_mercury, thread_safe, promise_pure],
>  "{
> -#define	PREDNAME			""functor/3""
> +/* shut up warning about argument NonCanon */

Delete that comment and s/NonCanon/_NonCanon/
Likewise below.

> +functor(_Term::in, _NonCanon::in(do_not_allow), _Functor::out,
> +		_Arity::out) :-
> +	error("NYI: deconstruct__functor/4").
> +functor(_Term::in, _NonCanon::in(canonicalize), _Functor::out,
> +		_Arity::out) :-
> +	error("NYI: deconstruct__functor/4").
> +functor(_Term::in, _NonCanon::in(include_details_cc), _Functor::out,
> +		_Arity::out) :-
> +	error("NYI: deconstruct__functor/4").
> +
> +univ_arg(_Term::in, _NonCanon::in(do_not_allow), _Index::in, _Arg::out) :-
> +	error("NYI: deconstruct__arg/4").
> +univ_arg(_Term::in, _NonCanon::in(canonicalize), _Index::in, _Arg::out) :-
> +	error("NYI: deconstruct__arg/4").
> +univ_arg(_Term::in, _NonCanon::in(include_details_cc), _Index::in, _Arg::out) :-
> +	error("NYI: deconstruct__arg/4").

"NYI" is quite cryptic.
It would be better to use sorry/1 from private_builtin.m,
or something along those lines.

> +% XXX
> +% deconstruct(Term::in, Functor::out, Arity::out, Arguments::out) :-
> +% 	rtti_implementation__deconstruct(Term, Functor, Arity, Arguments).

This should be explained or deleted.

> Index: library/io.m
...
> +:- pred io__print(io__output_stream, noncanon_handling, T,

I suggest s/noncanon_handling/deconstruct__noncanon_handling/

> Index: library/type_desc.m
...
> +	% same_type(X) = Y:
> +	% 	If X and Y have the same type at runtime (i.e. types T and U
> +	%	compare equal), then same_type, assigns X to Y. Otherwise,
> +	%	it fails.
> +:- func same_type(T) = U is semidet.

This is essentially the same as typed_unify in private_builtin.m.

As far as the interface goes, I think using a pred named typed_unify/2
makes for clearer code than using a semidet function.

> +:- pragma foreign_proc("C", 
> +	same_type(X::in) = (Y::out),
> +	[will_not_call_mercury, thread_safe, promise_pure],
> +"
> +	int	result;
> +
> +	MR_save_transient_hp();
> +	result = MR_compare_type_info((MR_TypeInfo) TypeInfo_for_T,
> +		(MR_TypeInfo) TypeInfo_for_U);
> +	MR_restore_transient_hp();
> +
> +	if (result == MR_COMPARE_EQUAL) {
> +		Y = X;
...
> +same_type(_X::in) = (_Y::out) :-
> +	error("NYI: same_type").

The implementation of typed_unify.m in private_builtin.m is more portable.

But rather than duplicating it, you should of course just call it.

> @@ -226,8 +226,16 @@
> +                MR_fatal_error(MR_STRINGIFY(EXPAND_FUNCTION_NAME)
> +                    ": attempt to deconstruct noncanonical term");

Please add an XXX comment somewhere: it would be nicer to throw an exception,
but we can't do that because it's not safe to throw exceptions
across the C interface.

> Index: tests/hard_coded/deconstruct_arg.m
> +set_to_sorted_list(Set) =
> +	promise_only_solution((pred(Sorted::out) is cc_multi :-
> +		Set = set_rep(Unsorted),
> +		list__sort(Unsorted, Sorted)
> +	)).

Shouldn't that be list__sort_and_remove_duplicates?

Actually it doesn't really matter for this test case;
what you've got is a perfectly valid multiset implementation.
But using the name "set" for a multiset could be misleading.

Otherwise that change looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------