[m-rev.] for review: RTTI/unify/compare for builtin types in the runtime

[Zoltan Somogyi]

On 08-Aug-2002, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> > +:- external(det_call_port_code_ac/3).
> > +:- external(det_call_port_code_sr/4).
> > +:- external(det_exit_port_code_ac/2).
> > +:- external(det_exit_port_code_sr/3).
> > +:- external(semi_call_port_code_ac/3).
> > +:- external(semi_call_port_code_sr/4).
> > +:- external(semi_exit_port_code_ac/2).
> > +:- external(semi_exit_port_code_sr/3).
> > +:- external(semi_fail_port_code_ac/2).
> > +:- external(semi_fail_port_code_sr/3).
> > +:- external(non_call_port_code_ac/4).
> > +:- external(non_call_port_code_sr/5).
> > +:- external(non_exit_port_code_ac/2).
> > +:- external(non_exit_port_code_sr/3).
> > +:- external(non_redo_port_code_ac/2).
> > +:- external(non_redo_port_code_sr/2).
> > +:- external(non_fail_port_code_ac/2).
> > +:- external(non_fail_port_code_sr/3).
> 
> It would be helpful to put a comment here explaining where they are defined.

Arghh...

It seems I had sent not just the pre-cleanup log, but a pre-cleanup diff as
well. On my laptop, there was already such a comment there.

> > +++ mercury_builtin_types.c	Sat Aug  3 20:49:28 2002
> ...
> > +
> > +/* unify and compare of type_ctor_infos are usually handled by generic code */
> 
> I don't understand that comment.
> (Likewise for similar ones after this.)

They are handled by the generic unify/2 and compare/3 predicates in
mercury_ho_call.c. These switch on the type_ctor_rep, and for most of the
builtin types, implement the unify or compare right there, *without* calling
the type-specific unify and compare routines.

I clarified those comments.

> > +#ifndef MERCURY_BUILTIN_H
> > +#define MERCURY_BUILTIN_H
> 
> The guard macro name should match the header file name.

Done.

> > +#define MR_DEFINE_PROC_STATICS(mod, n, a)                               \
> > +    MR_proc_static_compiler_empty(mod, __Unify__, n, a, 0,              \
> > +        MR_STRINGIFY(MR_PASTE2(mod, .m)), 0, MR_TRUE);                  \
> > +    MR_proc_static_compiler_empty(mod, __Compare__, n, a, 0,            \
> > +        MR_STRINGIFY(MR_PASTE2(mod, .m)), 0, MR_TRUE);
> 
> I think that is undefined behaviour, because `foo.m' is not a valid
> preprocessing token.  (The relevant sections in the C99 standard
> are 6.10.3.3 [#3] and 6.4 [#1], which I have quoted below.)
> 
> Rather than using token pasting, you can just use ANSI/ISO string
> concatenation:
> 
>     MR_proc_static_compiler_empty(mod, __Compare__, n, a, 0,               \
>         MR_STRINGIFY(mod) ".m", 0, MR_TRUE);

OK, I will do that.

> Incidentally, why do these strings include the ".m" anyway?

They are supposed to be filenames, with the 0 being a line number.

(The only meaningful context we could give is the context of the declarations
of these types, but only a few builtin types (e.g. type_desc) have even
abstract declarations; e.g. int is not declared anywhere.)

> In general, some more comments here, either in the generated code
> or sprinkled among the code which generates it, would be helpful.
> Why does the generated code have the form that it does?
> What is the function of this code?

I added a comment pointing readers to the deep profiling paper
in the script, in the literal string put into the generated files.

> > +                echo "#define MR_PROCNAME \"${msgname}\""
> > +                echo "#define MR_VERSION_${IMPL}"
> > +                echo "#define ${portdef}"
> > +                echo "${outermost_define} MR_NEED_NEW_OUTERMOST"
> > +                cat ${file}
> 
> Why not
> 		   echo "#include ${file}"

Because setting a breakpoint on line that is included several times in the
program doesn't do the right thing in gdb. I documented this fact at this line
in the script.

> > +# assembler the header file
> 
> s/assembler/assemble/ ?

Done.

> > -			-DMERCURY_BOOTSTRAP_H -DMERCURY_CONF_BOOTSTRAP_H
> > +			-DMERCURY_CONF_BOOTSTRAP_H
> >  MGNUCFLAGS	+= --no-ansi
> 
> This change disables the static checking that nothing in the
> trace directory depends on obsolete stuff in mercury_bootstrap.h.
> That static checking is important, IMHO.

I agree.

I originally thought that everything in mercury_bootstrap.h was disabled
already with a #ifdef, but I see now that is not true; the existing stuff
is protected only by a #ifndef MR_NO_BACKWARDS_COMPAT.

I will put -DMR_NO_BACKWARDS_COMPAT into the Mmakefiles that I removed
-DMERCURY_BOOTSTRAP_H from, to make sure that these see only the new,
temporary stuff.

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