[m-dev.] for review: cleanup of type_ctor_infos, part 0

[Fergus Henderson]

On 25-Feb-2000, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> |dead_proc_elim__eliminate_base_gen_infos([TypeCtorGenInfo0 | TypeCtorGenInfos0],
> |		Needed, TypeCtorGenInfos) :-
> |	dead_proc_elim__eliminate_base_gen_infos(TypeCtorGenInfos0, Needed,	
> |		TypeCtorGenInfos1),
> |	TypeCtorGenInfo0 = type_ctor_gen_info(TypeId, ModuleName,
> |		TypeName, Arity, Status, HldsDefn,
> |		_MaybeUnify, _MaybeIndex, _MaybeCompare,
> |		MaybeSolver, MaybeInit, MaybePretty),
>  	(
>  		Entity = base_gen_info(ModuleName, TypeName, Arity),
>  		map__search(Needed, Entity, _)
>  	->
> |		TypeCtorGenInfos = [TypeCtorGenInfo0 | TypeCtorGenInfos1]
>  	;
> |		NeuteredTypeCtorGenInfo = type_ctor_gen_info(TypeId,
> |			ModuleName, TypeName, Arity, Status, HldsDefn,
> |			no, no, no, MaybeSolver, MaybeInit, MaybePretty),
> |		TypeCtorGenInfos = [NeuteredTypeCtorGenInfo |
> |			TypeCtorGenInfos1]
>  	).

Why are MaybeSolver, MaybeInit, and MaybePretty treated
differently than MaybeUnify, MaybeIndex, and MaybeCompare here?

> +++ compiler/hlds_module.m	2000/02/22 05:55:24
> @@ -54,35 +54,22 @@
> +			hlds_type_defn,	% defn of type
> +			maybe(pred_proc_id),	% unif, if not eliminated
> +			maybe(pred_proc_id),	% inde, if not eliminated
> +			maybe(pred_proc_id),	% compare, if not eliminated
> +			maybe(pred_proc_id),	% solver, if relevant
> +			maybe(pred_proc_id),	% init, if relevant
> +			maybe(pred_proc_id)	% prettyprinter, if relevant

s/unif/unify/
s/inde/index/

>  :- type module_info
>  	--->	module(
> +			module_sub_info :: module_sub_info,
> +			pred_table :: predicate_table,

I strongly suggest s/pred_table/predicate_table/.
`pred_table' and `predicate_table' are two different types;
using the name `pred_table' for a `predicate_table' would misleading.

Clearly using the names `pred_table' and `predicate_table' for two
different types is itself confusing.  That one is my fault ;-)
On reflection, a better name for `predicate_table' would be
`indexed_pred_table'.  I'll make that change.

>  :- type module_sub_info
>  	--->	module_sub(
> +			module_name:: module_name,
> +			globals:: globals,
> +			c_header_info :: c_header_info,

For consistency, you should put spaces before all the `::'s.

> +			errors :: int,

I suggest s/errors/num_errors/

> +++ compiler/llds_out.m	2000/02/24 03:57:24
> @@ -18,6 +18,7 @@
>  :- interface.
>  
>  :- import_module llds, builtin_ops, prog_data, hlds_data, rl_file.
> +:- import_module llds_util, globals.
>  :- import_module set_bbbtree, bool, io, std_util.
>  
>  	% Given a 'c_file' structure, output the LLDS code inside it
> @@ -30,6 +31,48 @@
>  		io__state, io__state).
>  :- mode output_llds(in, in, in, di, uo) is det.
>  
> +:- pred output_rval_decls(rval, string, string, int, int, decl_set, decl_set,
> +	io__state, io__state).
> +:- mode output_rval_decls(in, in, in, in, out, in, out, di, uo) is det.

That predicate should be documented.

> +	% All the C data structures we generate which are either fully static
> +	% or static after initialization should have this prefix.
> +:- func mercury_data_prefix = string.

I don't understand why the identifier prefixing convention should
depend on whether the data is read-only or not.

> +:- pred output_data_addr_decls(data_addr::in, string::in, string::in,
> +	int::in, int::out, decl_set::in, decl_set::out,
> +	io__state::di, io__state::uo) is det.

That predicate should be documented.

> +	% Given the default linkage of a data item, and a bool sayinng whether
> +	% it is being defined, return a C string that gives its storage class.
> +
> +:- pred c_data_linkage_string(globals::in, linkage::in, bool::in, string::out)
> +	is det.

s/sayinng/saying/

> -% Every time we emit a declaration for a symbol, we insert it into the
> -% set of symbols we've already declared.  That way, we avoid generating
> -% the same symbol twice, which would cause an error in the C code.
> -
> -:- type decl_id --->	create_label(int)
> -		;	float_label(string)
> -		;	code_addr(code_addr)
> -		;	data_addr(data_addr)
> -		;	pragma_c_struct(string).
> -
> -:- type decl_set ==	map(decl_id, unit).
> -
> -:- pred decl_set_init(decl_set::out) is det.
> -
> -decl_set_init(DeclSet) :-
> -	map__init(DeclSet).
> -
> -:- pred decl_set_insert(decl_set::in, decl_id::in, decl_set::out) is det.
> -
> -decl_set_insert(DeclSet0, DeclId, DeclSet) :-
> -	map__set(DeclSet0, DeclId, unit, DeclSet).
> -
> -:- pred decl_set_is_member(decl_id::in, decl_set::in) is semidet.
> -
> -decl_set_is_member(DeclId, DeclSet) :-
> -	map__search(DeclSet, DeclId, _).

I think that change is not mentioned in the log message.

> @@ -2282,13 +2272,6 @@
>  	),
>  	io__write_string("struct "),
>  
> -	% If it's a type_ctor_info struct, use the MR_TypeCtorInfo_struct
> -	% type, and don't emit a definition.
> -	(
> -		{ decl_id_is_type_ctor_info(DeclId) }
> -	->
> -		io__write_string("MR_TypeCtorInfo_struct")
> -	;
>  		output_decl_id(DeclId),
>  		io__write_string("_struct"),
>  		(

It's not immediately clear to me that this change is right.
Probably it is, but perhaps you could explain why it is needed?

> -% output_code_addr_decls(CodeAddr, ...) outputs the declarations of any
> -% extern symbols, etc. that need to be declared before
> -% output_code_addr(CodeAddr) is called.
> -
> -:- pred output_code_addr_decls(code_addr, string, string, int, int,
> -	decl_set, decl_set, io__state, io__state).
> -:- mode output_code_addr_decls(in, in, in, in, out, in, out, di, uo) is det.
> -
>  output_code_addr_decls(CodeAddress, FirstIndent, LaterIndent, N0, N,
>  		DeclSet0, DeclSet) -->

Shouldn't the comment stay?

> +:- pred output_data_addr_scope_type_name(module_name::in, data_name::in,
> +	bool::in, string::in, io__state::di, io__state::uo) is det.

A comment explaining what this predicate is supposed to do would be helpful.
I don't quite understand what the `scope' in the predicate name means
here.

> -%
> -% Note that we need to know the linkage not just at the definition,
> -% but also at every use, because if the use is prior to the definition,
> -% then we need to declare the name first, and the linkage used in that
> -% declaration must be consistent with the linkage in the definition.
> -% For this reason, the field in c_data (which holds the information about
> -% the definition) which says whether or not a data name is exported
> -% is not useful.  Instead, we need to determine whether or not something
> -% is exported from its `data_name'.
> -%

Could you explain why that was comment deleted?

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