[m-rev.] for review: smart recompilation

[Fergus Henderson]

On 28-May-2001, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> Index: doc/user_guide.texi
> --- library/io.m	2001/05/02 14:44:26	1.224
...
> +:- pred io__file_modification_time(string, io__res(time_t),
> +		io__state, io__state).
> +:- mode io__file_modification_time(in, out, di, uo) is det.
> +	% io__file_modification_time(FileName, TimeResult)
> +	% finds the last modification time of the given file.
> +	% This predicate will only work on systems which provide
> +	% the C library function stat(). On other systems the
> +	% returned result will always be bound to error/1.

s/C/Posix C/

The interface with time_t and the documentation there are both overly
implementation-specific.
This should return an abstract type, e.g. `file_timestamp'.

The documentation should not require changing if we happen to
implement it on a non-Posix system (e.g. one that provides `_stat()'
or `GetFileTimeStamp()' rather than stat()).

> +:- pred io__input_stream_file_modification_time(io__input_stream,
> +		io__res(time_t), io__state, io__state).
> +:- mode io__input_stream_file_modification_time(in, out, di, uo) is det.

Is this really needed?

> +:- pred io__file_modification_time_2(string, int, string, time_t,
> +		io__state, io__state).
> +:- mode io__file_modification_time_2(in, out, out, out, di, uo) is det.
> +
> +:- pragma foreign_proc("C", io__file_modification_time_2(FileName::in,
> +		Status::out, Msg::out, Time::out, IO0::di, IO::uo),
> +		[will_not_call_mercury, thread_safe],
> +"{
> +#ifdef HAVE_STAT
> +	struct stat s;
> +	if (stat(FileName, &s) == 0) {
> +		Time = s.st_mtime;

Here you are assuming that a C time_t will fit into an MR_Word.
There should at least be an XXX comment.

> Index: runtime/RESERVED_MACRO_NAMES
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/RESERVED_MACRO_NAMES,v
> retrieving revision 1.2
> diff -u -u -r1.2 RESERVED_MACRO_NAMES
> --- runtime/RESERVED_MACRO_NAMES	2001/03/19 02:34:48	1.2
> +++ runtime/RESERVED_MACRO_NAMES	2001/05/28 06:20:26
> @@ -93,6 +93,7 @@
>  HAVE_SIGCONTEXT_STRUCT_3ARG
>  HAVE_SIGINFO
>  HAVE_SIGINFO_T
> +HAVE_STAT
>  HAVE_STRERROR
>  HAVE_SYSCONF
>  HAVE_SYS_PARAM

OK, but whoever is *next* to add to this file gets the job of
going through and changing them all to `MR_HAVE_*' ;-)

> Index: tests/recompilation/TESTS
> ===================================================================
> RCS file: TESTS
> diff -N TESTS
> --- /dev/null	Mon Apr 16 11:57:05 2001
> +++ TESTS	Mon May 28 15:29:29 2001
> @@ -0,0 +1,24 @@
> +
> +TESTS_SHOULD_SUCCEED="\
> +	add_constructor_r \
> +	add_instance_r \
> +	add_instance_2_r \
> +	add_type_nr \
> +	change_class_r \
> +	change_instance_r \
> +	change_mode_r \
> +	field_r \
> +	func_overloading_nr \
> +	func_overloading_r \
> +	lambda_mode_r \
> +	nested_module_r \
> +	no_version_numbers_r \
> +	pragma_type_spec_r \
> +	pred_ctor_ambiguity_r \
> +	pred_overloading_r"
> +
> +TESTS_SHOULD_FAIL="\
> +	add_type_re \
> +	remove_type_re \
> +	type_qual_re"

It would be very helpful to have a comment at the top of this file
explaining what the file is for; is this a shell script, a Makefile
fragment, or something else?  Perhaps the file should be renamed to
make this clear.

> --- /dev/null	Mon Apr 16 11:57:05 2001
> +++ add_constructor_r_2.err_exp.2	Tue May 22 17:36:07 2001
> @@ -0,0 +1,2 @@
> +Recompiling module `add_constructor_r_2':
> +  add_constructor_r_2.m has changed.

I suggest changing the message to

     source file `add_constructor_r_2.m' has changed.

You might also want to consider changing the ":\n  " to ",\n  because ".

> Index: tests/recompilation/add_instance_2_r.exp.1
> ===================================================================
> RCS file: add_instance_2_r.exp.1
> diff -N add_instance_2_r.exp.1
> --- /dev/null	Mon Apr 16 11:57:05 2001
> +++ add_instance_2_r.exp.1	Tue May 22 19:58:10 2001
> @@ -0,0 +1 @@
> +a
> \ No newline at end of file

Text files without newlines at the end are not completely portable.
You should change the program being tested so that it outputs a newline.

> Index: tests/recompilation/add_instance_2_r.exp.2
> Index: tests/recompilation/add_instance_r.exp.1
> Index: tests/recompilation/add_instance_r.exp.2
> Index: tests/recompilation/change_class_r.exp.1
> Index: tests/recompilation/change_class_r.exp.2
> Index: tests/recompilation/change_instance_r.exp.1
> Index: tests/recompilation/change_instance_r.exp.2
> Index: tests/recompilation/pragma_type_spec_r.exp.1
> Index: tests/recompilation/pragma_type_spec_r.exp.2

Likewise.

> --- /dev/null	Mon Apr 16 11:57:05 2001
> +++ add_type_re_2.err_exp.2	Mon May 28 15:15:20 2001
> @@ -0,0 +1,2 @@
> +Recompiling module `add_type_re_2':
> +  add_type_re_2.used: file not found.

For that message I suggest

     file `add_type_re_2.used' not found.

or

     recompilation dependency file `add_type_re_2.used' not found.

> Index: tests/recompilation/test_functions
> ===================================================================
> RCS file: test_functions
> diff -N test_functions
> --- /dev/null	Mon Apr 16 11:57:05 2001
> +++ test_functions	Mon May 28 17:34:28 2001
> @@ -0,0 +1,185 @@
> +		cp -f $module.m.1 $module.m

I'm not sure how portable the `-f' option to `cp' is.

> +mmake_depend () {
> +	if mmake $main_module.depend
> +	then
> +		:
> +	else 
> +		exit 1
> +	fi
> +}

I'd advise writing that more concisely, as

	mmake $main_module.depend || exit 1

But you should also pass $mmake_opts (which is set by handle_options)
to mmake.  (That also requires invoking it as `eval mmake ...' to get
the quoting right.)

> +#
> +# Compare the output file with the expected output file,
> +# generating the expected output file if it doesn't exist
> +# and the --generate-missing-exp-files option was given
> +# to runtests.
> +#
> +compare_files () {
> +	if [ $# != 2 ]
> +	then
> +		echo "usage: compare_files expected_file result_file"
> +		exit 1
> +	fi
> +	
> +	exp_file=$1
> +	res_file=$2
> +
> +	if [ -f $exp_file ]
> +	then
> +		if diff -c $exp_file $res_file >> $main_module.res

Rather than hard-coding `-c', it would be nicer to use `${DIFF_OPTS-"-c"}'.

> +		then
> +			:
> +		else
> +			exit 1
> +		fi

You should print out an error message before exiting.

> +		#
> +		# If the compilation is supposed to fail then the mmake
> +		# output should be suppressed to avoid making it harder
> +		# to find genuine failures in the nightly test logs.
> +		#
> +		mmake $main_module > /dev/null 2>&1

It would be better to put the output in a log file rather than /dev/null.

Also you should pass $mmake_opts to mmake, as is done in
the other tests/*/runtests scripts.

> +		;;
> +	    false)
> +		mmake $main_module

Likewise here.

> +		;;
> +	esac
> +
> +	case $? in
> +	    0)
> +		case $mmake_should_fail in
> +		    true)
> +			echo \
> +		"Error: mmake $main_module succeeded where it should fail"
> +			exit 1
> +			;;

Please make sure all error messages from test case failures start with "**",
so that we can easily grep for them in the test logs.

> +	    *)
> +		case $mmake_should_fail in
> +		    false)
> +			echo "Error: mmake $main_module failed"
> +			exit 1

Likewise here.

> +cleanup_test () {
> +	case $cleanup in
> +	true)
> +		mmake $main_module.realclean

Pass $mmake_opts.


I've reviewed everything except the new files compiler/recompilation*.m.

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