[m-dev.] for review: changes required for lcc

[Fergus Henderson]

On 16-Oct-1998, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> Changes required to get the samples directory to compiler using lcc in
> the grade none.gc

s/compiler/compile/

And while I'm being pedantic, s/none.gc/`none.gc'./

> compiler/llds_out.m:
>     Add a pass to make sure that the variables are all declared before
>     they are initialised.

Hmm, that log message doesn't seem quite right.  How about

     Add a pass to make sure that the struct types are all defined
     before they are used.  This is necessary because ANSI C doesn't
     provide any way of forward declaring static variables with
     incomplete type.

instead?

>     Also change data_ptr to type (Word *).  This is because lcc doesn't
>     like casts between const and non-const, when doing certain
>     initialisations.

Is that really true?  Or do you mean to say that lcc doesn't like implicit
conversions between (Word *) and (const Word *)?  I thought that lcc
will allow it if you use an explicit cast.

Note that there are two possible ways of solving this problem:
change both LHS and RHS to be (Word *), or change both LHS and RHS
to be (const Word *).  The log message doesn't really explain
why you chose the first solution rather than the second one.
Using (const Word *) would seem to be more truthful, at least
for the case of pointers to static constants, which was I think
the one causing the problems with lcc, wasn't it?

I noticed a comment in llds.m about data_ptr being `const Word *'
that would be made obsolete by this change; you should fix that.
Also there are some uses of `const Word *' in fact_table.m, as well as
a lot in llds_out.m.  There are also some in library/builtin.m and lots
in runtime/*.[ch].  Probably you should do a grep for `const Word'
and check all those places to see whether they still make sense.

> Index: compiler/llds_out.m
> +	%
> +	% output_c_data_def_list outputs all the type definitions of
> +	% the module.  This is needed because some compilers need the
> +	% definition to appear before its declaration.
> +	%

I suggest you change the last line to

	% data definition to appear before any use of the type
	% in forward declarations of static constants.

> +	% Also we now conditionally output some parts according to
> +	% whether we are outputting the definition, declaration or both.
>  
> +:- pred output_const_term_decl(list(maybe(rval)), decl_id, bool, bool, bool, 
> +		bool, string, string, int, int, io__state, io__state).
> +:- mode output_const_term_decl(in, in, in, in, in,
> +		in, in, in, in, out, di, uo) is det.

The new comment explains two of the new `bool' parameters, but what
is the meaning of the third?

Also it's not clear here whether `definition' and `declaration' 
refer to the declaration/definition of the struct type or the
declaration/definition of the constant which has that type.

> +output_const_term_decl(ArgVals, DeclId, Exported, Def, Decl, Init, FirstIndent, 
> +		LaterIndent, N1, N) -->
...
> +		globals__io_get_globals(Globals),
> +		{ globals__have_static_code_addresses(Globals, StaticCode) },
> +		(
> +			{ StaticCode = no },
> +			{ DeclId = data_addr(
> +					data_addr(_, base_type(info, _, _))) }
> +		->
> +			[]
> +		;
> +			io__write_string("const ")
> +		)

You should put a comment there explaining why base_type_infos are treated
specially.

> @@ -3033,7 +3129,7 @@
>  output_rval_const(string_const(String)) -->
>  		% XXX we should change the definition of `string_const'
>  		% so that this cast is not necessary
> -	io__write_string("(const Word *) string_const("""),
> +	io__write_string("(Word *) string_const("""),

You changed the definition of `string_const' so that there was no cast.
I think you should either instead change the definition of `string_const'
so that it casts to `Word *', or delete the above `XXX' comment --
probably the former would be better.

>  	output_c_quoted_string(String),
>  	{ string__length(String, StringLength) },
>  	io__write_string(""", "),
> @@ -3048,7 +3144,8 @@
>  output_rval_const(data_addr_const(data_addr(ModuleName, VarName))) -->
>  	% data addresses are all assumed to be of type `Word *';
>  	% we need to cast them here to avoid type errors
> -	io__write_string("(const Word *) &"),
> +	% XXX No we don't to make lcc accept it.
> +	io__write_string("(Word *) &"),

I think the XXX comment here is misleading; the data addresses
still  need to be cast, it's just that now they need to be cast to
`Word *' rather than `const Word *'.  I suggest you just delete
the XXX comment.

>  /*
> -** the result of mkword() is cast to (const Word *), not to (Word)
> +** the result of mkword() is cast to (Word *), not to (Word)
>  ** because mkword() may be used in initializers for static constants
>  ** and casts from pointers to integral types are not valid
> -** constant-expressions in ANSI C.
> +** constant-expressions in ANSI C.  It cannot be (const Word *) because
> +** some ANSI C compilers won't allow assignments where the RHS is of type
> +** const and the LHS is not declared const.

You mean "where the RHS is of type pointer to const and the LHS is
of type pointer to non-const".

Also you can do
	s/some ANSI C compilers won't allow/ANSI C does not allow/

--------------------

OK, apart from the comments I made above, it looks good.
I'd like to see another diff though.
I'd also like to see a bit more rationale for the change to data_ptr,

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