[m-dev.] for review: abstract instance declarations

[David Glen JEFFERY]

Thanks for doing this one Fergus. Sorry for not reviewing it sooner, but I
haven't been online for the last two weeks.

This diff looks generally fine. I have just a couple of comments.

On 09-Feb-1999, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> Estimated hours taken: 9
> 
> Implement abstract instance declarations.
> 	[...]

> 
> -		% there are no methods for this class
> -		InstanceDefn0 = hlds_instance_defn(A, B, C, D, E,
> -				_MaybeInstancePredProcs, G, H),
> -		InstanceDefn1 = hlds_instance_defn(A, B, C, D, E,
> -				yes([]), G, H),
> -		ModuleInfo1 = ModuleInfo0
> +			ClassId = class_id(ClassName, ClassArity),
> +			prog_out__sym_name_to_string(ClassName,
> +				ClassNameString),
> +			string__int_to_string(ClassArity, ClassArityString),
> +			string__append_list([
> +				"In instance declaration for `",
> +				ClassNameString, "/", ClassArityString, "': ",
> +				"incorrect method name(s)."],
> +				NewError),
> +			Errors2 = [Context - [words(NewError)] | Errors1]
> +		)

Is this a new error message? I'm not sure that "incorrect method name(s)" is
the right thing to say.

> +:- pred check_for_overlapping_instances(hlds_instance_defn,
> +		list(hlds_instance_defn), class_id, io__state, io__state).
> +:- mode check_for_overlapping_instances(in, in, in, di, uo) is det.
> +
> +check_for_overlapping_instances(NewValue, Values, ClassId) -->
> +	{ IsOverlapping = lambda([(Context - OtherContext)::out] is nondet, (
> +		NewValue = hlds_instance_defn(_Status, Context,
> +				_, Types, Body, _, VarSet, _),
> +		Body \= abstract, % XXX
> +		list__member(OtherValue, Values),
> +		OtherValue = hlds_instance_defn(_OtherStatus, OtherContext,
> +				_, OtherTypes, OtherBody, _, OtherVarSet, _),
> +		OtherBody \= abstract, % XXX
> +		varset__merge(VarSet, OtherVarSet, OtherTypes,
> +				_NewVarSet, NewOtherTypes),
> +		type_list_subsumes(Types, NewOtherTypes, _)
> +	)) },
> +	aggregate(IsOverlapping,
> +		report_overlapping_instance_declaration(ClassId)).

`Values' isn't a very good variable name in this context. (It's OK at the
point where you call check_for_overlapping_instances because at that point
it is quite clear that the `Values' are the values from the instance map.
Inside the call, though, the name doesn't make much sense).

> +report_overlapping_instance_declaration(class_id(ClassName, ClassArity),
> +		Context - OtherContext) -->
> +	io__set_exit_status(1),
> +	prog_out__write_context(Context),
> +	io__write_string("Error: multiply defined (or overlapping) instance\n"),

Could it actually be overlapping, given that we restrict instance types to be
a single functor with variable args?

> +Each @samp{instance} declaration
> +must specify a binding for every method declared in the corresponding
> + at samp{class} declaration. 

Is "binding" the right word? The word I normally use is "implementation", but
I'm not sure that it's quite correct. This sentence should probably also state
that there must be *exactly* one implementation of each method. Also, the
declaration is "typeclass", not "class". Maybe:

	Each @samp{instance} declaration
	must specify exactly one implementation for every method declared in the
	corresponding @samp{typeclass} declaration. 

BTW, there are plenty of places in this diff where "typeclass" is used where
"type class" should be. Almost all of those places were just existing mistakes
that you didn't fix. I will make a pass over the reference manual (one of
these days...) and fix all of them.


dgj
-- 
David Jeffery (dgj at cs.mu.oz.au) |  Marge: Homer, is this how you pictured
PhD student,                    |         married life?
Dept. of Comp. Sci. & Soft. Eng.|  Homer: Yup, pretty much... except we
The University of Melbourne     |         drove around in a van solving
Australia                       |         mysteries.