[m-rev.] for review: smart recompilation

[Simon Taylor]

On 04-Jun-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> On 28-May-2001, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> > Index: compiler/equiv_type.m
> > +	% 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.

Done.
 
> > 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)])'?

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

Fixed.

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

Fixed.
 
> > +	% 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/

Done.

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

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

+               % This call is only used to make higher_order.m generate
+               % the interface for the type specialized procedure, and
+               % will be removed by higher_order.m after that is done.
 
> > @@ -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?

Ambiguity with the field extraction function for the `modes' field of
`recompilation__item_id_set'.
 
> > 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) },

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

Done.

> > @@ -656,15 +888,27 @@
> > +		% 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.

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

We shouldn't be encouraging interactive use of `mmc', especially with
`--smart-recompilation'. `--sr' could be confused with structure reuse.
 
> > 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/

Done.
 
> > +:- 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?

Yes.

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

Fixed.

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