[m-rev.] for review: mmc --make [4]

[Fergus Henderson]

On 06-Feb-2002, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> Index: compiler/make.util.m
...
> +	% Timestamp handling.
> +
> +	% Find the timestamp updated when a target is produced.
> +:- pred get_timestamp_file_timestamp(target_file::in,
> +	maybe_error(timestamp)::out, make_info::in, make_info::out,
> +	io__state::di, io__state::uo) is det.
> +
> +:- pred get_dependency_timestamp(dependency_file::in,
> +	maybe_error(timestamp)::out, make_info::in, make_info::out,
> +	io__state::di, io__state::uo) is det.
> +
> +:- pred get_target_timestamp(target_file::in, maybe_error(timestamp)::out,
> +	make_info::in, make_info::out, io__state::di, io__state::uo) is det.
> +
> +:- pred get_file_name(target_file::in, file_name::out,
> +	make_info::in, make_info::out, io__state::di, io__state::uo) is det.
> +
> +:- pred get_file_timestamp(list(dir_name)::in, file_name::in,
> +	maybe_error(timestamp)::out, make_info::in, make_info::out,
> +	io__state::di, io__state::uo) is det.

These routines should be documented (in the interface).

> +%-----------------------------------------------------------------------------%
> +	% Remove file a file, deleting the cached timestamp.
> +
> +:- pred remove_target_file(target_file::in, make_info::in, make_info::out,
> +	io__state::di, io__state::uo) is det.
> +
> +:- pred remove_target_file(module_name::in, module_target_type::in,
> +	make_info::in, make_info::out, io__state::di, io__state::uo) is det.
> +
> +	% remove_file(ModuleName, Extension, Info0, Info).
> +:- pred remove_file(module_name::in, string::in, make_info::in, make_info::out,
> +		io__state::di, io__state::uo) is det.
> +
> +:- pred remove_file(file_name::in, make_info::in, make_info::out,
> +		io__state::di, io__state::uo) is det.

Likewise.  What is the difference between these similarly-named routines?

> +volatile sig_atomic_t MC_signal_received = 0;
> +
> +void
> +MC_mercury_compile_signal_handler(int sig)
> +{
> +	MC_signalled = TRUE;
> +	MC_signal_received = sig;

Hmm... here you are assuming that a signal number can fit into a
sig_atomic_t, which is not guaranteed to be more than 8 bits, according
to the C standard.  So I don't think this is guaranteed to work.

It's probably going to be portable, though.
Maybe just add an XXX.

> +:- pred setup_signal_handlers(signal_action::out,
> +		io__state::di, io__state::uo) is det.
> +
> +:- pragma foreign_proc("C",
> +		setup_signal_handlers(SigintHandler::out, IO0::di, IO::uo),
> +		[will_not_call_mercury, promise_pure],

You should have (mode-specific) Mercury clauses as alternatives for any
procedures defined in C code, so that it compiles with non-C back-ends.

Also it might be nicer to isolate the stuff that needs to be
implemented in C in a separate module, e.g. say make.process_util.m.

> +	% call_io_pred(P, ExitStatus).
> +:- pred call_io_pred(io_pred::in(io_pred), int::out,
> +		io__state::di, io__state::uo) is det.
> +:- pragma export(call_io_pred(in(io_pred), out, di, uo), "call_io_pred").

C functions exported from the Mercury compiler should have an `MC_' prefix,
so s/call_io_pred/MC_call_io_pred/

> +	% Note that we need a timestamp file for `.err' files because
> +	% errors is written to the `.err'. The timestamp is only updated
> +	% when compiling to target code.

s/errors is/errors are/

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