[m-dev.] for review: MLDS backend to do structure reuse and compile time gc

[Fergus Henderson]

On 06-Oct-2000, Peter Ross <peter.ross at miscrit.be> wrote:
> @@ -1057,12 +1059,11 @@
>  unop_code(tag,			 1).
>  unop_code(unmktag,		 2).
>  unop_code(mkbody,		 3).
> -unop_code(body,			 4).
> -unop_code(unmkbody,		 5).
> -unop_code(cast_to_unsigned,	 6).
> -unop_code(hash_string,		 7).
> -unop_code(bitwise_complement,	 8).
> -unop_code((not),		 9).
> +unop_code(unmkbody,		 4).
> +unop_code(cast_to_unsigned,	 5).
> +unop_code(hash_string,		 6).
> +unop_code(bitwise_complement,	 7).
> +unop_code((not),		 8).

That change will break the code in bytecode/disasm.c.
I think you need to make the following change to that file
to make it work:

Index: disasm.c
===================================================================
RCS file: /home/mercury1/repository/mercury/bytecode/disasm.c,v
retrieving revision 1.14
diff -u -d -u -r1.14 disasm.c
--- disasm.c	1997/07/27 14:59:21	1.14
+++ disasm.c	2000/10/07 14:06:39
@@ -587,7 +587,8 @@
 	"float_lt",
 	"float_gt",
 	"float_le",
-	"float_ge"
+	"float_ge",
+	"body"
 };
 
 static const char*
@@ -610,7 +611,6 @@
 	"tag",
 	"unmktag",
 	"mkbody",
-	"body",
 	"unmkbody",
 	"cast_to_unsigned",
 	"hash_string",

> +++ ml_unify_gen.m	2000/10/06 09:56:26
> -ml_gen_unification(deconstruct(Var, ConsId, Args, ArgModes, CanFail),
> +
> +ml_gen_unification(deconstruct(Var, ConsId, Args, ArgModes, CanFail, CanCGC),
> +	(
> +			% Note that we can deallocate a cell even if the 
> +			% unification fails, it is the responsibility of
> +			% the structure reuse phase to ensure that this
> +			% is safe.
> +		{ CanCGC = yes },

Please don't indent the comments.  The style used throughout the rest
of that file, and in the examples showing layout of comments in the
Mercury coding guidelines, is for comments to be intended to the
same level as the code that they apply to.

> +	% Calculate the integer offset used to reference the first field
> +	% of a structure for lowlevel data or the first argument number
> +	% to access the field using the highlevel data representation.
> +	% Abort if the tag indicates that the data doesn't have any
> +	% fields.
> +:- pred ml_tag_offset_and_argnum(cons_tag::in, tag_bits::out,
> +		int::out, int::out) is det.
> +
> +ml_tag_offset_and_argnum(Tag, TagBits, OffSet, ArgNum) :-
> +	(
> +		Tag = unshared_tag(UnsharedTag),
> +		TagBits = UnsharedTag,
> +		OffSet = 0,
> +		ArgNum = 1
> +	;
> +		Tag = shared_remote_tag(PrimaryTag, _SecondaryTag),
> +		TagBits = PrimaryTag,
> +		OffSet = 1,
> +		ArgNum = 1
> +	;
> +		Tag = string_constant(_String),
> +		error("ml_tag_offset_and_argnum")

Here's one I missed the first time around.
Why have the `ArgNum' argument in this predicate?
The documentation doesn't say what the `ArgNum' field is for.
The code always returns ArgNum = 1 (if it returns at all).
Having the ArgNum argument here makes it confusing.

Looking at the code elsewhere, I see that ArgNum is set to 1 to
initialize the loop counter for ml_gen_unify_args.  I think it would
be better style to put `1' explicitly in the calls to
ml_gen_unify_args.  (Even better would be to define ml_gen_unify_args
without the ArgNum argument, and have it then just call
ml_gen_unify_args_2 with ArgNum = 1.)

Apart from that, this change looks fine.

-- 
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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------