[m-dev.] For Review: Bytecode interpreter

[Fergus Henderson]

On 25-Jan-2001, Levi Cameron <l.cameron2 at ugrad.unimelb.edu.au> wrote:
> Fergus Henderson wrote:
> > > Index: mb_bytecode.c
> > > @@ -540,10 +597,14 @@
> > >                               bc_p->opt.call.pred_id =
> > >                                       pred_id;
> > >                               bc_p->opt.call.arity = arity;
> > > +                             bc_p->opt.call.is_func = is_func;
> > >                               bc_p->opt.call.proc_id = proc_id;
> > > +
> > > +                             bc_p->opt.call.is_native = TRUE;
> > > +                             bc_p->opt.call.adr = NULL;
> > >                               return TRUE;
> > 
> > Why do you set is_native = TRUE here?
> > 
> > That won't always be the case, so there should at least be a comment
> > explaining why you set it to TRUE here.
> 
> It is supposed to cause an error if you ever try to execute it.
> Changed to is_native = FALSE; adr = MB_CODE_INVALID_ADR
> which should be a bit clearer

OK.  A comment would also help.

> /*
> ** Returned string is allocated with string routines MB_str_xxx
> ** It is the responsibility of the caller to free it using MB_str_delete
> */
> static MB_Bool
> MB_read_cstring(FILE *fp, MB_CString *str_p)
> {
> 	/*
> 	** Initially tries to read string into buffer, but if this gets
> 	** full then mallocs another buffer which is doubled in size
> 	** whenever it gets full.
> 	** Returned string is allocated with MB_str_dup
> 	*/
> 	char	buffer[64];
> 	MB_Word	bufsize = sizeof(buffer);
> 	char*	str = buffer;
> 	
> 	MB_Word		i = 0;
> 	MB_Byte		c;
> 	
> 	do {
> 		/* get the next char */
> 		if (!MB_read_byte(fp, &c)) {
> 			MB_fatal("Error reading C String from file");
> 		}
> 
> 		/*
> 		** If the next char is going to overflow the buffer then
> 		** expand the buffer
> 		*/
> 		if (i == bufsize) {
> 			if (str == buffer) {
> 				/*
> 				** If we are still using the stack buffer,
> 				** allocate a new buffer with malloc
> 				*/
> 				str = MB_malloc(bufsize*2);
> 				memcpy(str, buffer, bufsize);
			} else {

I think you are missing bufsize *= 2 there.

> > > +                     /* Whether call is a native code call */
> > > +                     MB_Byte         is_native;
> > 
> > Why is that MB_Byte rather than `bool'?
> 
> Changed to MB_Bool which is MB_Byte (and in all the other places
> is_xxx is used)-- why is MR_Bool defined as the same size as an
> int? 32 bits seems somewhat wasteful when 1 will do.

MR_Bool is probably a bad name; we should call it `MR_Mercury_Bool'
instead.  That type is designed to be layout-compatible with
Mercury's `bool' type, i.e. the same size as `MR_Word'.

For booleans in C code, we use `bool' rather than `MR_Bool', unless
there is some particular reason why they need to be layout-compatible
with Mercury's bool.  `bool' is a #define for `char'.
(We should probably make that one a typedef and rename it as `MR_Bool'.)

> > Your mailer seems to be wrapping long lines.
> > You should turn that off when posting diffs.
> 
> It was wrapping at 72 (who set that?)

That's a quite common default, since it means that lines can be quoted
with "> " a few times without going over 80 columns.

> > >               case MB_CONSID_INT_CONST:
> > > -                     /*
> > > -                     ** (This comment is labelled "CAST COMMENT".
> > > -                     ** If you remove this comment, also
> > > -                     ** remove references to it in this file.
> > > -                     ** Search for "CAST COMMENT".)
> > > -                     **
> > > -                     ** XXX: The cast to `long' in the following code
> > > -                     ** is needed to remove a warning. `int_const' has
> > > -                     ** type `Integer', but Integer may be typedef'ed
> > > -                     ** to `int', `long', `long long' or whatever.
> > > -                     ** The correct solution may be to define a
> > > -                     ** format string for Integer in conf.h.
> > > -                     */
> > >                       Print()
> > > -                             "int_const %ld",
> > > -                             (long) cons_id.opt.int_const
> > > +                             "int_const %x",
> > > +                             (int)cons_id.opt.int_const
> > 
> > Why did you change the cast from int to long?
> > And why did you remove the XXX comment?  It still applies, I think.
> > 
> > Why do you print int constants in hex rather than decimal?
> > 
> > If you are going to print in hex, you should definitely
> > precede it with "0x" to make it clear that the output is
> > in hex.  Otherwise output like "10" would be ambiguous.
> > 
> > If you think hex output would be useful, it may be better
> > to output both decimal _and_ hex, e.g. "int_const 16 (0x10)".
> 
> I misinterpreted the comment. Changed now to:
> 
> 				/*
> 				** XXX: int_const has type Integer which could
> 				** be int, long or long long. Correct solution
> 				** is to define a format string in conf.h, but
> 				** for now assume long is enough
> 				*/
> 				"int_const %ld (%lx)",
> 				(long) cons_id.opt.int_const,
> 				(long) cons_id.opt.int_const
> (and duplicated at str_op_arg > MB_ARG_INT_CONST)

There is in fact a format string defined in runtime/mercury_conf.h now,
it's MR_INTEGER_LENGTH_MODIFIER.  So probably you should just use that.

> > > +     if (MB_ip_special(new_ip)) {
> > > +             switch ((MB_Word)new_ip) {
> > > +                     case (MB_Word)MB_CODE_DO_FAIL:
> > > +                             instr_do_fail(ms, NULL);
> > > +                             break;
> > > +                     case (MB_Word)MB_CODE_DO_REDO:
> > > +                             instr_do_redo(ms, NULL);
> > > +                             break;
> > 
> > Why do you need the casts to MB_Word here?
> 
> C doesn't allow you to switch on a pointer (as I discovered)

Ah, I see ;-)

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- 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
--------------------------------------------------------------------------