[m-rev.] for review: expand equivalence types fully

[Fergus Henderson]

On 29-Nov-2003, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> Make definitions of abstract types available when generating
> code for importing modules.  This is necessary for the .NET
> back-end, and for `:- pragma export' on the C back-end.

Great, thanks!

> Index: compiler/mercury_to_mercury.m
...
>  :- pred mercury_output_begin_type_decl(is_solver_type, io__state, io__state).
>  :- mode mercury_output_begin_type_decl(in, di, uo) is det.
>  
> -mercury_output_begin_type_decl(solver_type) -->
> -	io__write_string(":- solver type ").
> -mercury_output_begin_type_decl(non_solver_type) -->
> -	io__write_string(":- type ").
> +mercury_output_begin_type_decl(IsSolverType) -->
> +	io__write_string(":- "),
> +	( { IsSolverType = solver_type }, io__write_string("solver ")
> +	; { IsSolverType = non_solver_type }
> +	),
> +	io__write_string("type ").

IMHO the original code is considerably easier to read.

> Index: compiler/module_qual.m
...
> +:- pred add_imports_2(list(sym_name)::in, mq_info::in, mq_info::out) is det.
> +
> +add_imports_2(Imports, !Info) :-
> +	mq_info_get_import_status(!.Info, Status),
> +	(
> +		( Status = local 
> +		; Status = exported
> +		; Status = imported(ancestor_private_interface)
>  		)
> +	->
> +		mq_info_get_imported_modules(!.Info, Modules0),
> +		set__insert_list(Modules0, Imports, Modules),
> +		mq_info_set_imported_modules(!.Info, Modules, !:Info)
>  	;
> -		Info = Info0
> -	).
> +		true
> +	),
> +
> +	(
> +		Status = exported
> +	->
> +		mq_info_get_unused_interface_modules(!.Info,
> +			UnusedIntModules0),
> +		set__insert_list(UnusedIntModules0, Imports, UnusedIntModules),
> +		mq_info_set_unused_interface_modules(!.Info,
> +			UnusedIntModules, !:Info)
> +	;
> +			true
> +	),
> +
> +	(
> +		( Status = exported
> +		; Status = imported(ancestor_private_interface)
> +		)
> +	->
> +		mq_info_get_interface_visible_modules(!.Info, IntModules0),
> +		set__insert_list(IntModules0, Imports, IntModules),
> +		mq_info_set_interface_visible_modules(!.Info,
> +			IntModules, !:Info)
> +	;
> +		true
> +	).	

Some comments there would be helpful.

>  
>  %------------------------------------------------------------------------------
>  
> @@ -513,13 +585,16 @@
>  module_qualify_item(clause(A,B,C,D,E) - Con, clause(A,B,C,D,E) - Con,
>  			Info, Info, yes) --> [].
>  
> -module_qualify_item(type_defn(A, SymName, Params, TypeDefn0, C) - Context,
> -		type_defn(A, SymName, Params, TypeDefn, C) - Context,
> +module_qualify_item(
> +		type_defn(TVarSet, SymName, Params, TypeDefn0, C) - Context,
> +		type_defn(TVarSet, SymName, Params, TypeDefn, C) - Context,
>  		Info0, Info, yes) --> 
> +	{ mq_info_get_types(Info0, Types0)  },
>  	{ list__length(Params, Arity) },
>  	{ mq_info_set_error_context(Info0,
>  		type(SymName - Arity) - Context, Info1) },
> -	qualify_type_defn(TypeDefn0, TypeDefn, Info1, Info).  
> +	qualify_type_defn(TypeDefn0, TypeDefn, Info1, Info2),
> +	{ mq_info_set_types(Info2, Types0, Info) }. 

I don't understand why you reset the types field to its old value there.
A comment might help.

> @@ -1226,6 +1314,28 @@
>  		mercury_output_bracketed_sym_name(ModuleName),
>  		io__write_string("' has not been imported).\n")
>  	;
> +		{ MatchingModules = [_ | MatchingModules1] }
> +	->
> +		{ MatchingModules1 = [],
> +			ModuleWord = "module ",
> +			HasWord = "has"
> +		; MatchingModules1 = [_|_],
> +			ModuleWord = "modules ",
> +			HasWord = "have"
> +		},
> +
> +		io__write_string("\n"),
> +		prog_out__write_context(Context),
> +		io__write_string("  (the "),
> +		io__write_string(ModuleWord),
> +		io__write_string(" "),
> +		prog_out__write_module_list(MatchingModules),
> +		io__nl,
> +		prog_out__write_context(Context),
> +		io__write_string("  "),
> +		io__write_string(HasWord),
> +		io__write_string(" not been imported in the interface).\n")
> +	;

That looks like it outputs two spaces after "module" or "modules",
but I think it should only output one.

There should be a test case for both variations (singular and plural)
of this new error message.

> Index: compiler/module_qual.m
> @@ -1418,6 +1528,10 @@
>  	set__init(InterfaceModules0),
>  	get_implicit_dependencies(Items, Globals, ImportDeps, UseDeps),
>  	set__list_to_set(ImportDeps `list__append` UseDeps, ImportedModules),
> +	set__insert_list(ImportedModules,
> +		[ModuleName | get_ancestors(ModuleName)],
> +		InterfaceVisibleModules),

Why do the ancestor modules need to get included in InterfaceVisibleModules
here?

> Index: compiler/modules.m
> @@ -520,60 +523,114 @@
>  		item_list, module_imports, module_error, io__state, io__state).
>  :- mode grab_unqual_imported_modules(in, in, in, in, out, out, di, uo) is det.
>  
> -	% process_module_private_interfaces(Ancestors, DirectImports0,
> -	%			DirectImports, DirectUses0, DirectUses,
> -	%			Module0, Module):
> +	% process_module_private_interfaces(Ancestors,
> +	%	IntStatusItem, ImpStatusItem, DirectImports0, DirectImports,
> +	%	DirectUses0, DirectUses, Module0, Module):

I suggest using state var notation here, i.e.

	% process_module_private_interfaces(Ancestors,
	%	IntStatusItem, ImpStatusItem, !DirectImports, !DirectUses,
	%	!Module):

> @@ -1422,6 +1484,139 @@
> +:- pred strip_unnecessary_impl_defns(item_list::in, item_list::out) is det.
> +
> +strip_unnecessary_impl_defns(Items0,
> +	promise_only_solution(strip_unnecessary_impl_defns_2(Items0))).

There should be a comment explaining why that promise is safe.

> @@ -1782,7 +1979,7 @@
>  
>  update_interface_create_file(Msg, OutputFileName,
>  			TmpOutputFileName, Succeeded) -->
> -	globals__io_lookup_bool_option(verbose, Verbose),
> +	globals__io_lookup_bool_option(verbose_make, Verbose),

I don't think that was mentioned in the log message.
It appears to be unrelated to the rest of your changes
and hence ideally should be committed separately (if at all).

> @@ -6509,6 +6839,43 @@
>  include_in_short_interface(module_defn(_, _)).
>  include_in_short_interface(instance(_, _, _, _, _, _)).
>  
> +:- func item_needs_imports(item) = bool.

A comment explaining the meaning of this function would be helpful.

> +:- pred include_in_int_file_implementation(item).
> +:- mode include_in_int_file_implementation(in) is semidet.
> +
> +include_in_int_file_implementation(type_defn(_, _, _, _, _)).
> +include_in_int_file_implementation(module_defn(_, Defn)) :-
> +	Defn \= external(_).
> +include_in_int_file_implementation(typeclass(_, _, _, _, _)).

It would be nice to put a comment explaining why typeclass declarations
need to be included, but instance declarations don't.

> +++ compiler/prog_data.m	26 Nov 2003 08:46:41 -0000
> @@ -56,8 +56,15 @@
>  			cl_body			:: goal
>  		)
>  
> -		% `:- type ...':
> +		% `:- type ...' or `:- type_impl':
>  		% a definition of a type, or a declaration of an abstract type.
> +		% The compiler places `:- type_impl' declarations in `.int'
> +		% and `.int2' files to propagate definitions of abstract
> +		% foreign types and equivalence types which should not be
> +		% visible to importing modules (and shouldn't be used when
> +		% typechecking those modules), but which are necessary for
> +		% code generation.  For examples the MS Common Language Runtime
> +		% doesn't support equivalence types.
>  	; 	type_defn(

Is the comment about ":- type_impl" correct?
It doesn't match the description in the log message.

> @@ -1234,6 +1233,11 @@
>  		% `:- use_module', and items from `.opt'
>  		% and `.int2' files. It also records from which
>  		% section the module was imported.
> +	;	abstract_imported
> +		% This is used internally by the compiler,
> +		% to identify items which originally
> +		% came from the implementation section
> +		% of an interface file.

It would be good to just mention here what implementation sections
in interface files are for.

>  	;	opt_imported
>  		% This is used internally by the compiler,
>  		% to identify items which originally
> @@ -1270,7 +1274,8 @@
>  :- type import_locn
>  	--->	implementation
>  	;	interface
> -	;	ancestor.
> +	;	ancestor
> +	;	ancestor_private_interface.

It would be helpful to have a comment here explaining what that new
alternative means.

> Index: compiler/recompilation.usage.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/recompilation.usage.m,v
> retrieving revision 1.10
> diff -u -u -r1.10 recompilation.usage.m
> --- compiler/recompilation.usage.m	24 Oct 2003 06:17:47 -0000	1.10
> +++ compiler/recompilation.usage.m	26 Nov 2003 04:45:01 -0000
> @@ -79,6 +79,7 @@
>  %-----------------------------------------------------------------------------%
>  :- implementation.
>  
> +:- import_module check_hlds.
>  :- import_module check_hlds__type_util.
>  :- import_module hlds__hlds_data.
>  :- import_module hlds__hlds_out.

IMHO that declaration should go in recompilation.m
rather than here and in recompilation.version.m.

> +++ compiler/notes/compiler_design.html	24 Nov 2003 14:33:23 -0000
> @@ -689,6 +689,20 @@
>  
>  (Is there any good reason why lambda.m comes after table_gen.m?)
>  
> +
> +<p>
> +
> +Expansion of equivalence types (equiv_type_hlds.m)
> +
> +<ul>
> +<li>
> +	This pass expands equivalences which are not meant to
> +	be visible to the user of imported modules.  This
> +	is necessary for the IL back-end and in some cases
> +	for `:- pragma export' involving foreign types on
> +	the C back-end.
> +</ul>

It's also needed by the MLDS->C back-end, for either --high-level-data,
or for cases involving abstract equivalence types which are defined
as "float".

> @@ -104,17 +104,29 @@
>  :- mode multi_map__values(in, out) is det.
>  
>  	% convert a multi_map to an association list
> -:- pred multi_map__to_assoc_list(multi_map(K,V), assoc_list(K,list(V))).
> +:- pred multi_map__to_assoc_list(multi_map(K,V), assoc_list(K, V)).
>  :- mode multi_map__to_assoc_list(in, out) is det.

I don't think that this is worth breaking backwards compatibility for.
The new predicate could be named e.g. multi_map__to_flat_assoc_list.
If you think the old name is confusing, you could rename it and keep
a version with the original name but use pragma obsolete to deprecate it.

> +++ tests/hard_coded/Mercury.options	28 Nov 2003 12:24:05 -0000
> @@ -4,6 +4,7 @@
>  MCFLAGS-constraint	=	--constraint-propagation --enable-termination
>  MCFLAGS-constraint_order =	--constraint-propagation --enable-termination
>  MCFLAGS-deforest_cc_bug =	--deforestation
> +MCFLAGS-export_test2	=	--no-intermodule-optimization

Is that needed?  The test will normally run without intermodule optimization
anyway.  Is there anything wrong with also testing it with intermodule
optimization occaisionally?

> +++ tests/invalid/missing_interface_import.err_exp	25 Nov 2003 15:48:55 -0000
> @@ -1,9 +1,14 @@
>  missing_interface_import.m:007: In definition of type `missing_interface_import.bar'/0:
> -missing_interface_import.m:007:   error: undefined type `map'/2.
> +missing_interface_import.m:007:   error: undefined type `map'/2
> +missing_interface_import.m:007:   (the module  `map'
> +missing_interface_import.m:007:   has not been imported in the interface).

s/module  `map'/module `map'/

>  missing_interface_import.m:009: In definition of predicate `missing_interface_import.p'/1:
>  missing_interface_import.m:009:   error: undefined type `std_util.univ'/0
> -missing_interface_import.m:009:   (the module `std_util' has not been imported).
> +missing_interface_import.m:009:   (the module  `std_util'
> +missing_interface_import.m:009:   has not been imported in the interface).

Why is there a line wrap before the "has not been ..."?
Is that deliberate, or accidental?

> Index: NEWS

The changes to that file were not mentioned in the log message and look
like they do not belong as part of this change.

> Index: library/multi_map.m

The changes to this file were not mentioned in the log message.

Ideally the changes to multi_map.m should be handled as a separate change
rather than as part of this diff.


Otherwise, this diff looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------