[m-dev.] For review: Add implementation of reference types (global heap)

[Fergus Henderson]

On 09-Jun-1998, Warwick Harvey <wharvey at cs.monash.edu.au> wrote:
> Here's the updated diffs, as requested...
> +Word
> +MR_make_permanent(Word term, Word *type_info)
> +{
> +	return MR_make_long_lived(term, type_info, NULL);
> +}

It would be more efficient to define this as a macro in the header file.
Same applies for MR_make_long_lived() in the CONSERVATIVE_GC case.

You can take that as an optional suggestion ;-)

> +#define swap(val1, val2, type) \
> +	do { \
> +		type swap_tmp; \
> +		swap_tmp = (val1); \
> +		(val1) = (val2); \
> +		(val2) = swap_tmp; \
> +	} while (0)

Since this macro is not function-like (its first two arguments
are by reference and the last is a type), I suggest s/swap/SWAP/.

Also it's easier to read if you line up all the backslashes.

> +++ mercury_deep_copy.h	1998/06/09 06:48:37
> @@ -63,4 +63,32 @@
>  Word deep_copy(Word data, Word *type_info, Word *lower_limit, 
>  	Word *upper_limit);
>  
> +/* MR_make_permanent:
> +**

Should be

	/*
	** ...
	**

not

	/* ...
	**

> +/* MR_make_long_lived:
> +**

Ditto.

> extras/references/nb_reference.m
...
> :- implementation.
> 
> %  This type is not really used.  I'd rather define this as c_pointer, but
> %  there's a bug in the Mercury compiler that makes that not work.
> :- type nb_reference(T) ---> nb_reference(c_pointer).

The first sentence in that comment is a bit misleading.
I suggest you replace it with "This type is implemented in C."

Regarding the second sentence,
It's true that there is a bug in the Mercury compiler that makes

	:- type nb_reference(T) == c_pointer.

not work.  However, the definition using == is actually less useful
than the one above, IMHO, since the one above gives you more informative
output from type_of/1, io__write/3, etc.
Thus I suggest you delete the second sentence.

> :- pragma inline(new_nb_reference/2).
> :- pragma c_code(new_nb_reference(X::in, Ref::out), will_not_call_mercury, "
> 	incr_hp(Ref, 1);
> 	save_transient_registers();
> 	*(Word *)Ref = MR_make_long_lived(X, (Word *) TypeInfo_for_T,
> 			(Word *) Ref);
> 	restore_transient_registers();
> ").
> 
> :- pragma inline(value/2).
> :- pragma c_code(value(Ref::in, X::out), will_not_call_mercury, "
> 	X = *(Word *) Ref;
> ").
> 
> :- pragma inline(update/2).
> :- pragma c_code(update(Ref::in, X::in), will_not_call_mercury, "
> 	save_transient_registers();
> 	*(Word *)Ref = MR_make_long_lived(X, (Word *) TypeInfo_for_T,
> 			(Word *) Ref);
> 	restore_transient_registers();
> ").

If you were concerned about efficiency, you could wrap `#ifndef
CONSERVATIVE_GC ... #endif' around those calls to
save_transient_registers() and restore_transient_registers().

> extras/references/reference.m
...
> :- implementation.
> 
> %  This type is not really used.  I'd rather define this as c_pointer, but
> %  there's a bug in the Mercury compiler that makes that not work.
> :- type reference(T) ---> reference(c_pointer).

Same comment as above.

> :- pragma inline(new_reference/2).
> :- pragma c_code(new_reference(X::in, Ref::out), will_not_call_mercury, "
> 	incr_hp(Ref, (sizeof(Reference) + sizeof(Word) - 1) / sizeof(Word));
> 	((RefPtr)Ref)->value = (void *)X;
> 	((RefPtr)Ref)->id = MR_current_choicepoint_id();
> ").
>
> :- pragma inline(value/2).
> :- pragma c_code(value(Ref::in, X::out), will_not_call_mercury, "
> 	X = (Word) ((RefPtr)Ref)->value;
> ").

Our convention is to put a space after type casts:

	s/(RefPtr)/(RefPtr) /g
	s/(void *)/(void *) /

> :- pragma c_code(update(Ref::in, X::in), will_not_call_mercury, "
> 	RefPtr ref = (RefPtr) Ref;
> 	if (ref->id != MR_current_choicepoint_id()) {
> 		MR_trail_current_value((Word*)(&ref->value));
> 		MR_trail_current_value((Word*)(&ref->id));
> 		ref->id = MR_current_choicepoint_id();
> 	}
> 	ref->value = (void *)X;
> ").

Ditto here.

> typedef struct {
> 	Word *var;
> 	Word insideval;
> 	Word outsideval;
> } *ScopeHandle;
> 
> void ME_enter_scope_failing(ScopeHandle handle, MR_untrail_reason reason);
> void ME_exit_scope_failing(ScopeHandle handle, MR_untrail_reason reason);

The ScopeHandle type should get a `ME_' prefix too, lest it clash
with a type defined in some other header file.

(There is some argument for prefixing even struct members, lest
they clash with macro names, but I think that the inconvenience
of the longer names probably outweighs the advantage in that case.
Defining lower-case non-function-like macros has always been considered
bad style in C, so if the user does that, and it causes a clash, then
I think it is not unreasonable for us to say that it's up to the user
to fix it.)

> #ifdef ME_DEBUG_SCOPE
> #define show_handle(msg, handle) \
> 	printf(""%s <%5d, in: %5d, out: %5d\n"", msg, *(int *)handle->var, \
> 			(int) handle->insideval, (int) handle->outsideval)
> #define untrail_msg(msg) \
> 	printf(msg)
> #else
> #define show_handle(msg, handle)
> #define untrail_msg(msg)
> #endif

These macros should get a ME_ prefix too.
Also, the uses of the macro parameters should be parenthesized.
And our convention is that stuff inside #ifdef should be intended two spaces:

#ifdef ME_DEBUG_SCOPE
  #define ME_show_handle(msg, handle) 					\
	printf(""%s <%5d, in: %5d, out: %5d\n"", (msg),			\
			*(int *)(handle)->var,				\
			(int) (handle)->insideval,			\
			(int) (handle)->outsideval)
  #define ME_untrail_msg(msg)						\
	printf(msg)
#else
  #define ME_show_handle(msg, handle)
  #define ME_untrail_msg(msg)
#endif

I think these changes should be straightforward, but just to confirm,
can you please send one more diff when you've dealt with them?
I'd prefer a relative diff (i.e. make a backup copy of your current
version of the files before you modify them and diff against that)
this time.

Thanks again,
	Fergus.

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