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

[Peter Ross]

Before I address all your comments, just one point.  This diff is meant
to be one that compiles everything in the samples directory.  It is not
the complete solution, but it does bootstrap using *gcc* without any
warnings.

I would prefer to make the changes in an incremental fashion, and check
them in.

On 16-Oct-1998, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> 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'./
> 
ok.

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

It is actually that they must be defined and declared before they are
initialised.

ie.

struct s {
    int *x;
} y;

struct s y = { &z };

> >     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 chose the second solution because (const Word *) is a subset of (Word *),
and that it is a nontrivial task to ensure that the const get outputed
in the correct places.

Static constants do cause the problems.  I attempted to place the code
in to output the const's in the correct place.  However that was causing
problems with bootstraping using gcc.

I will attempt to output const's in the correct place after I get while
I try to get the compiler to bootstrap using lcc.


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

I will do this will attempting to bootstrap the compiler using lcc.

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

> > +	% 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?
> 

The third parameter is whether the initialisation code is output for
the variable.  ie the '= { &z };'

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

I am not sure what you are trying to say.  Each constant has a unique
type.  So I think the answer is it is for both.

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

I have no idea.  I just put the if-then-else bits around the code.

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

I will delete the XXX.  I left it in because someone might remember why
the XXX was there and tell me I had done the wrong thing.

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

ok.

> >  /*
> > -** 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,

The rationale is above, and the rest of it is really delete a few lines,
and change some of the comments.

Do you still want to see a diff?

Pete.