[m-dev.] for review: generating C layout structures

[Fergus Henderson]

----- Forwarded message from Zoltan Somogyi <zs at cs.mu.OZ.AU> -----

Date: Thu, 18 Jan 2001 18:03:24 +1100
From: Zoltan Somogyi <zs at cs.mu.OZ.AU>
To: Fergus Henderson <fjh at cs.mu.OZ.AU>
Subject: Re: [m-dev.] for review: generating C layout structures
User-Agent: Mutt/1.2.5i

On 18-Jan-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> You committed your change, but you didn't reply to the following
> review message.  Obviously you read it, since you addressed some of
> the review comments (but not others).  Could you please reply to this
> (on mercury-developers), explaining which review comments were
> addressed, and how, and which ones were ignored, and why?

I followed all your suggestions, with two exceptions.

> > There seems to be a lot of code duplication there.
> > I think it would be better to factor out most of it
> > into separate subroutines:
> > 
> > 	output_layout_addr_storage_type_name(Layout, _BeingDefined) -->
> > 		( layout_name_is_exported(LayoutName) = yes ->
> > 			io__write_string("static ")
> > 		;
> > 			io__write_string("extern ")
> > 		),
> > 		( layout_name_would_include_code_addr(LayoutName) = yes ->
> > 			io__write_string("MR_STATIC_CODE_CONST ")
> > 		;
> > 			io__write_string("const ")
> > 		),
> > 		io__write_string(layout_type(Layout)),
> > 		output_layout_name(Layout),
> > 		( layout_name_has_array_type(Layout) = yes ->
> > 			io__write_string("[]"
> > 		;
> > 			[]
> > 		).

I ignored this, because 

1 - The code above does not express the meaning of the code you suggest it
    replace. For example, including code addresses is not the only reason
    why a layout structure may not be const; it may contain information
    derived during a profiling run, for example.

2 - It is buggy: it needs to call extract_layout_name to turn Layout
    into LayoutName.

3 - The total size of this predicate and its auxiliaries is about the same
    as the size of the original code.

> > This is actually an existing problem, not one introduced
> > by this change, so you don't need to address it in this change.
> > But since it was another of your changes that introduced this problem,
> > it would be nice if you could fix it.

I ignored this for this change.

> > > +	closure_id->module_name = ""dl"";
> > > +	closure_id->file_name = ""dl.m"";
> > > +	closure_id->line_number = ++MR_dl_closure_counter;
> > > +	closure_id->goal_path = """";
> > 
> > Why do you set the line number in this fashion?
> > Wouldn't it be more accurate to set it to the current line,
> > like this?

This is the only suggestion which I adopted for which the adoption is not
straightforward: I used the goal path field to uniquely identify the closure
instead of the line number field.

Zoltan.

----- End forwarded message -----

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