[m-dev.] for review: rewrite of check_typeclass.m

[Fergus Henderson]

On 20-May-1998, David Glen JEFFERY <dgj at cs.mu.OZ.AU> wrote:
> @@ -129,11 +196,20 @@
>                                  ClassProc = hlds_class_proc(PredId, ProcId)
>                          )),
>                  ProcIds),
> -        module_info_pred_info(ModuleInfo, PredId, PredInfo),
> -        pred_info_arg_types(PredInfo, _ArgTypeVars, ArgTypes),
> -        pred_info_name(PredInfo, PredName0),
> +        module_info_pred_info(ModuleInfo0, PredId, PredInfo),
> +        pred_info_arg_types(PredInfo, ArgTypeVars, ArgTypes),
> +        pred_info_get_class_context(PredInfo, ClassContext0),
> +        (
> +                ClassContext0 = [_|Tail]
> +        ->
> +                ClassContext = Tail
> +        ;
> +                error("check_instance_pred: no constraint on class method")
> +        ),

You should put a comment explaining why you strip off the first
constraint in the class context.

> +                % Work out what the name of the predicate that we generate
> +                % to check this instance method should be.

I suggest you delete the words "what" and "should be"
and insert "will" in front of "generate".

> +:- pred check_instance_pred_procs(list(var), sym_name,
> +        hlds_instance_defn, hlds_instance_defn, 
> +        instance_method_info, instance_method_info).
> +:- mode check_instance_pred_procs(in, in, in, out, in, out) is det.
> +
> +check_instance_pred_procs(ClassVars, MethodName, InstanceDefn0, InstanceDefn, 
I don't understand why you still need this predicate,
and many others in this module.  Is that just to support the optimization

> +                % As an optimisation, if the types and constraints
> +                % are _exactly_ the same, there is no point introducing
> +                % a predicate to call the instance method

?  I think you could simplify the code dramatically
if you delete this optimization -- is that correct?

I think the right time to do this optimization is after LLDS optimization.
Doing it here seems to add a lot of complexity for relatively little benefit.
This will have significant maintenance costs.

I suggest you check it in with the optimization in place,
and then delete the optimization as a separate change.
(That way, if we ever want to add it back, it'll be easy.)

Oh, I suppose deleting the optimization is not a high priority item.

>  %---------------------------------------------------------------------------%
> +        % Make the name of the introduced pred used to check a particular
> +        % instance of a particular class method
> +        %
> +        % XXX This isn't quite perfect, I suspect
> +:- pred make_introduced_pred_name(class_id, sym_name, arity, list(type), 
> +        sym_name).
> +:- mode make_introduced_pred_name(in, in, in, in, out) is det.
> +
> +make_introduced_pred_name(ClassId, MethodName, _PredArity, 
> +                InstanceTypes, PredName) :-
> +        ClassId = class_id(ClassName, _ClassArity),
> +        unqualify(ClassName, ClassNameString),
> +        unqualify(MethodName, MethodNameString),
> +                % Perhaps we should include the pred arity in this mangled
> +                % string?
> +        % string__int_to_string(PredArity, PredArityString),
> +        base_typeclass_info__make_instance_string(InstanceTypes, 
> +                InstanceString),
> +        string__append_list(
> +                ["Introduced predicate for ",
> +                ClassNameString,
> +                "(",
> +                InstanceString,
> +                ") method ",
> +                MethodNameString
> +                ], 
> +                PredNameString),
> +        PredName = unqualified(PredNameString).

The pred name had better be unique, otherwise you may get
link errors or C compiler errors.  Thus yes, you should include
the pred arity.  You should also s/predicate/function/ for functions.

> +:- pred unqualify(sym_name::in, string::out) is det.
> +
> +unqualify(unqualified(X), X).
> +unqualify(qualified(M0, X), Result) :-
> +        unqualify(M0, M),
> +        string__append_list([M, "__", X], Result).

Use `prog_out__sym_name_to_string'.

-- 
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.