[m-rev.] for review: smart recompilation

[Fergus Henderson]

On 28-May-2001, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> Index: compiler/equiv_type.m
> -	% equiv_type__expand_eqv_types(Items0, Items, CircularTypes, EqvMap).
> +	% equiv_type__expand_eqv_types(ModuleName, Items0, Items,
> +	%	CircularTypes, EqvMap, MaybeRecompInfo0, MaybeRecompInfo).
>  	%
>  	% First it builds up a map from type_id to the equivalent type.
>  	% Then it traverses through the list of items, expanding all types. 
>  	% This has the effect of eliminating all the equivalence types
>  	% from the source code. Error messages are generated for any
>  	% circular equivalence types.
> +	%
> +	% For items not defined in the current module, record the
> +	% equivalence types expanded while processing each item
> +	% in the recompilation_info.

It might be useful to mention here that this is done because it is needed
for the --smart-recompilation option.

> Index: compiler/error_util.m
...
> +	% Convert a list of lists of format_components into a list of
> +	% format_components, suitable for displaying as an error message.
> +:- func error_util__component_lists_to_pieces(list(list(format_component))) =
> +		list(format_component).

That comment doesn't give enough information about the semantics of
this function to understand what it does without looking at the code.
How does it differ from list__condense?

The name of this routine is rather long and it would be nice if we could
think of something more concise that clearly conveyed what it does.

> +:- func error_util__append_punctuation(list(format_component), char) =
> +		list(format_component).

A comment explaining what this routine does would help.
How does it differ from say `func(L,C) = append(L, [words(C)])'?

> +error_util__append_punctuation([], _) = _ :-
> +	error(
> +	"error_util__append_full_stop: appending punctuation after nothing").

The procedure name in the call to error/1 is wrong.

> Index: compiler/handle_options.m
> +++ compiler/handle_options.m	2001/05/28 05:38:52

> +	%
> +	% Disable `--smart-recompilation' for compilation options
> +	% which either do not produce a compiled output file or
> +	% for which smart recompilation will not work.
> +	%
> +	option_implies(generate_dependencies, smart_recompilation, bool(no)),
> +	option_implies(convert_to_mercury, smart_recompilation, bool(no)),
> +	option_implies(make_private_interface, smart_recompilation, bool(no)),
> +	option_implies(make_interface, smart_recompilation, bool(no)),
> +	option_implies(make_short_interface, smart_recompilation, bool(no)),
> +	option_implies(make_short_interface, smart_recompilation, bool(no)),

The line above is duplicated.

In the long run it would be nice to support --smart-recompilation for
--make-interface.

> +	% XXX Smart recompilation does not yet work with
> +	% `--no-target-code-only'. With `--no-target-code-only'
> +	% it becomes difficult to work out what all the target
> +	% files are and check whether they are up-to-date.
> +	% By default, mmake always enables `--target-code-only' and
> +	% processes the object file itself, so this isn't a problem.

s/object file/target code file/

> Index: compiler/intermod.m
>  add_item_clause(module_defn(_, Defn), Status0, Status, _,
> -		Module, Module, Info0, Info) --> 
> -	{ module_defn_update_import_status(Defn, ItemStatus1) ->
> +		Module0, Module, Info0, Info) --> 
> +	{ Defn = version_numbers(ModuleName, ModuleVersionNumbers) ->
> +		apply_to_recompilation_info(
> +		    (pred(RecompInfo0::in, RecompInfo::out) is det :-
> +			map__set(RecompInfo0 ^ version_numbers, ModuleName,
> +				ModuleVersionNumbers, VersionNumbers),
> +			RecompInfo = RecompInfo0 ^ version_numbers
> +					:= VersionNumbers
> +		    ),
> +		    transform_info(Module0, Info0),
> +		    transform_info(Module, Info)),

I found that code a little hard to read. 
I suggest using `map__elem' rather than map__set:

		    (pred(RecompInfo0::in, RecompInfo::out) is det :-
			RecompInfo = RecompInfo0 ^ version_numbers ^
				map__elem(ModuleName) := ModuleVersionNumbers
		    ),

Adding a comment might help too.

> @@ -958,8 +962,10 @@
>  		goal_info_set_nonlocals(GoalInfo0, NonLocals, GoalInfo1),
>  		goal_info_set_context(GoalInfo1, Context, GoalInfo),
>  
> -		construct_pred_or_func_call(PredId, PredOrFunc, SymName, Args,
> -			GoalInfo, Goal),
> +		% We don't record the called predicate as used --
> +		% it is only used if there is some other call.
> +		do_construct_pred_or_func_call(PredId, PredOrFunc, SymName,
> +			Args, GoalInfo, Goal),

I think it would be helpful if you could elaborate on that comment a bit.
Why doesn't this use count?

> @@ -3871,7 +3782,7 @@
>  
>  add_annotation(empty, no, none).
>  add_annotation(empty, yes(Mode), modes([Mode])).
> -add_annotation(modes(_), no, mixed).
> +add_annotation(modes(_ `with_type` list(mode)), no, mixed).
>  add_annotation(modes(Modes), yes(Mode), modes(Modes ++ [Mode])).
>  add_annotation(none, no, none).
>  add_annotation(none, yes(_), mixed).

Why is the explicit type annotation needed here?

> Index: compiler/mercury_compile.m

> +:- pred read_module(file_or_module, bool, module_name, file_name,
> +		maybe(time_t), item_list, module_error,
> +		read_modules, read_modules, io__state, io__state).
> +:- mode read_module(in, in, out, out, out, out, out, in, out, di, uo) is det.
>  
> -process_module_name(ModuleName, ModulesToLink) -->
> +read_module(module(ModuleName), ReturnTimestamp, ModuleName, FileName,
> +		MaybeTimestamp, Items, Error, ReadModules0, ReadModules) -->

What's the meaning of the `ReadModules0' and `ReadModules' arguments here?

> +:- pred process_module_2(file_or_module, modules_to_recompile,
> +		read_modules, list(string), io__state, io__state).
> +:- mode process_module_2(in, in, in, out, di, uo) is det.
> +
> +process_module_2(FileOrModule, MaybeModulesToRecompile, ReadModules0,
> +		ModulesToLink) -->
> +	read_module(FileOrModule, yes, ModuleName, FileName,
> +		MaybeTimestamp, Items, Error, ReadModules0, ReadModules),
> +	globals__io_lookup_bool_option(halt_at_syntax_errors, HaltSyntax),
> +	( { halt_at_module_error(HaltSyntax, Error) } ->
> +		{ ModulesToLink = [] }
> +	;
> +		split_into_submodules(ModuleName, Items, SubModuleList0),
> +		{ MaybeModulesToRecompile = some(ModulesToRecompile) ->
> +			list__filter(
> +				(pred((SubModule - _)::in) is semidet :-
> +					list__member(SubModule,
> +						ModulesToRecompile)
> +				),
> +				SubModuleList0, SubModuleList)
> +		;
> +				SubModuleList = SubModuleList0	
> +		},

Hmm... this isn't going to work for the `--target asm' back-end, I think.
For that back-end, you need to recompile all of the sub-modules if you
recompile any of them, since they get compiled into a single object file.

> +		{ assoc_list__keys(SubModuleList, InlineSubModules0) },
> +		{ list__delete_all(InlineSubModules0,
> +			ModuleName, InlineSubModules) },

I suggest s/InlineSubModules/NestedSubModules/g, here and elsewhere,
to be consistent with the terminology used in the language reference
manual and elsewhere in the compiler.

> @@ -656,15 +888,27 @@
>  		globals__io_lookup_bool_option(target_code_only, 
>  				TargetCodeOnly),
>  
> +		%
> +		% Remove any existing `.used' file before writing the
> +		% output file file. This avoids leaving the old `used'
> +		% file lying around if compilation is interrupted after
> +		% the new output file is written but before the new
> +		% `.used' file is written.
> +		%
> +		module_name_to_file_name(ModuleName, ".used", no,
> +			UsageFileName),
> +		invoke_system_command("rm -f " ++ UsageFileName, _),

You should use io__remove_file rather than invoke_system_command.

> Index: compiler/modules.m
...
> +:- pred read_mod_if_changed(module_name, string, string, bool, time_t,
> +		item_list, module_error, file_name, maybe(time_t),
> +		io__state, io__state).
> +:- mode read_mod_if_changed(in, in, in, in, in,
> +		out, out, out, out, di, uo) is det.

There should be a comment for that procedure.

Also you should minimize the number of places which assume that file
timestamps have type time_t; use an abstract type here.

> Index: compiler/options.m

There should probably be a shorter alternative name for the
`--smart-recompilation' option, e.g. `--sr', for interactive use.

> Index: compiler/prog_data.m

You should document the new invariants for the `prog_data__type'
type here.

...
> @@ -44,42 +45,31 @@
>  :- type item_and_context ==	pair(item, prog_context).
>  
>  :- type item		
...
> +	; 	type_defn(tvarset, sym_name, list(type_param),
> +			type_defn, condition)
> +		%	VarNames, Name, Args, TypeDefn, Condition

I suggest s/Name, Args/TypeName, Args/

> +:- type item_warning
> +	--->	item_warning(
> +			maybe(option),	% Option controlling whether the
> +					% warning should be reported.
> +			string,		% The warning.
> +			term		% The term to which it relates.
> +		).

Why do you include the maybe(option) here?
Is that because the globals isn't being passed into the place which
produces the warning?

> Index: compiler/prog_io.m
> @@ -246,11 +267,44 @@
>  		{ dir__this_directory(CurrentDir) },
>  		{ Dirs = [CurrentDir] }
>  	),
> +	io__input_stream(OldInputStream),
>  	search_for_file(Dirs, FileName, R),
>  	( { R = yes } ->
> -		read_all_items(DefaultModuleName, ModuleName,
> -			Messages, Items, Error),
> -		io__seen
> +		( { ReturnTimestamp = yes } ->
> +			io__input_stream(InputStream),
> +			io__input_stream_file_modification_time(InputStream,
> +				TimestampResult),
> +			(
> +				{ TimestampResult = ok(Timestamp) },
> +				{ MaybeModuleTimestamp = yes(Timestamp) } 
> +			;
> +				{ TimestampResult = error(_) },
> +				{ MaybeModuleTimestamp = no }

You shouldn't throw away the error status (`_') here, if this is going
to result in an error message or warning message later.

At very least that warrants an XXX comment.

[... to be continued ...]

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