[m-dev.] for review: introduction of two new directories

[Fergus Henderson]

On 24-Sep-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Index: Mmake.common.in
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/Mmake.common.in,v
> retrieving revision 1.28
> diff -u -u -r1.28 Mmake.common.in
> --- Mmake.common.in	1998/08/04 14:05:50	1.28
> +++ Mmake.common.in	1998/09/24 04:41:23
> @@ -59,6 +59,7 @@
>  INSTALL_PS_DIR 		= $(INSTALL_PREFIX)/lib/mercury/doc
>  INSTALL_MAN_DIR 	= $(INSTALL_PREFIX)/man
>  INSTALL_HTML_DIR 	= $(INSTALL_PREFIX)/lib/mercury/html
> +INSTALL_DOC_DIR 	= $(INSTALL_PREFIX)/lib/mercury/doc
>  
>  # Specify the Mercury compiler to use for bootstrapping
>  MC			= @BOOTSTRAP_MC@

Should that be part of this change?

> +
> +# The names of the various libraries.
> +# The archives and shared object objects have a "lib" prefix and a ".a" or
> +# ".so" suffix around these names;

Actually that is system dependent.  On Windows they have a ".dll" suffix.
I suggest you just add "(or .dll)" to the comment.

> +# If you change these, you will also need to change script/ml.in

s/script/scripts/

>  .PHONY: compiler
> -compiler: dep_compiler scripts util runtime boehm_gc library
> +compiler: dep_compiler util scripts boehm_gc runtime library browser trace

Any particular reason for swapping the order of `util' and `scripts' there?

>  .PHONY: install_main
>  install_main: all $(PREINSTALL_HACK) \
>  		install_scripts install_util install_runtime install_boehm_gc \
> -  		install_library install_compiler install_profiler install_doc \
> +  		install_library install_browser install_trace \
> +		install_compiler install_profiler install_doc \

Any particular reason for installing the `trace' library before
installing the `browser' library?

> +		( cd browser && \
> +		  $(SUBDIR_MMAKE) GRADE=$$grade install_library ) && \
> +		( cd trace && \
> +		  $(SUBDIR_MMAKE) GRADE=$$grade install_lib ) && \

Likewise.

> Index: browser/Mmakefile
...
> +# browser/Mmakefile - this is the Mmakefile for building the Mercury
> +# browser library, which also includes other functionality needed
> +# by Mercury debuggers.
> +
> +# Since the code in this directory is intended to be invoked only from
> +# the trace library, which turns off tracing in the Mercury code it calls,
> +# compiling the modules in this directory with tracing on only makes
> +# makes the generated code much bigger.

Delete one of the two occurrences of the word `makes'.

> However, since all Mercury code
> +# in an executable must be of the same grade, we need to be able to
> +# compile the modules in this directory in debug grades as well.

How about compiling this directory with `--trace minimal', then?
That would minimize the overhead.

> Index: browser/browser.m
...
> +:- module browser.
> +
> +:- interface.
> +
> +:- implementation.
> +
> +:- import_module help.

This will get a warning, since there's nothing in the interface section.

If this is compiled with `--halt-at-warn', as it should be, then the
warning will be treated as an error.

One possible solution would be to use nested modules:

	:- module browser.
	:- interface.
	:- include_module help.

> Index: browser/help.m

Hmm, I'm wondering about the name "browser" now.
Is this new directory supposed to contain a term browser, a help browser,
or both?

If the answer is "both", then "browsers" would be a better name.
But it might be better still to use a more inclusive name, such
as "debugger".

> Index: doc/Mmakefile
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/doc/Mmakefile,v
> retrieving revision 1.11
> diff -u -u -r1.11 Mmakefile
> --- Mmakefile	1998/09/01 05:17:38	1.11
> +++ Mmakefile	1998/09/16 04:48:02
> @@ -47,7 +47,7 @@
>  #-----------------------------------------------------------------------------#
>  
>  .PHONY: all
> -all: info dvi html manpages # ps text
> +all: info dvi html manpages mdb_doc # ps text

That shouldn't be part of this change, should it?

Same question applies to all the other changes to doc/Mmakefile.

> Index: runtime/Mmakefile
> @@ -45,6 +45,7 @@
>  			mercury_imp.h		\
>  			mercury_init.h		\
>  			mercury_label.h		\
> +			mercury_layout_util.h	\
>  			mercury_library_types.h	\
>  			mercury_memory.h	\
>  			mercury_memory_zones.h	\
> @@ -112,6 +109,7 @@
>  			mercury_heap_profile.c	\
>  			mercury_ho_call.c	\
>  			mercury_label.c		\
> +			mercury_layout_util.c	\
>  			mercury_memory.c	\
>  			mercury_memory_zones.c	\
>  			mercury_memory_handlers.c	\

Should that be part of this change?

> @@ -253,11 +258,12 @@
>  #
>  # .debug grade implies --use-trail
>  #	(see comment in compiler/handle_options.m for rationale)
> +#	(... and for why it does not work yet)
>  #
> -case $stack_trace,$require_tracing in
> -	true,true)
> -		use_trail=true ;;
> -esac
> +# case $stack_trace,$require_tracing in
> +# 	true,true)
> +# 		use_trail=true ;;
> +# esac

That should definitely not be part of this change.

> @@ -366,13 +372,25 @@
>  
>  case $mercury_libs in
>  	shared)
> -		LIBS=${MERCURY_LIBS="-lmercury -lmer $LIBGC $STDLIBS"}
> +		# Repeating -l$STD_LIB_NAME for shared objects would have
> +		# no effect.
> +		LIBS=${MERCURY_LIBS="-l$TRACE_LIB_NAME -l$BROWSER_LIB_NAME -l$STD_LIB_NAME -l$RT_LIB_NAME $LIBGC $STDLIBS"}

Any particular reason why $BROWSER_LIB_NAME comes before $TRACE_LIB_NAME?

> +	static)
> +		# We repeat lib$STD_LIB_NAME.a. The first occurrence is to
> +		# pick up objects needed by the program. Since the library
> +		# may have been compiled with tracing even though the program
> +		# isn't, this may then introduce references to MR_trace, which
> +		# will pull in the browser library and possibly other objects
> +		# from the standard library.

I thought the main purpose for introducing the two new directories
was to avoid problems with cyclic dependencies between libraries.  But from
the above, it sounds like there may be cyclic dependencies anyway, i.e.
adding the new directories didn't solve the problem.  Is that correct?

> Index: tools/bootcheck
>  
> +RT_LIB_NAME=mer_rt
> +STD_LIB_NAME=mer_std
> +TRACE_LIB_NAME=mer_trace
> +BROWSER_LIB_NAME=mer_browser

I've seen those four lines of code in three places now.  I think you
should at least add a comment in all 3 places saying something along
the lines of "if you change this here, you may need to change it in
these other two places".

> +	# We need to give tests/debugger access to the mdbrc and mdb_doc
> +	# files in the doc directory, without hardcoding their pathnames.
> +	# We must also compensate for doc/mdbrc having hardcoded within it
> +	# the *installed* pathname of mdb_doc and not its current pathname.
> +	cat $root/doc/mdb_doc > $root/doc/test_mdbrc
> +	sed -e '/^source/d' $root/doc/mdbrc >> $root/doc/test_mdbrc
> +	MERCURY_DEBUGGER_INIT=$root/doc/test_mdbrc
> +	export MERCURY_DEBUGGER_INIT

Should that be part of this change?

trace/Mmakefile:
> +# keep this list in alphabetical order, please
> +HDRS		=	\
> +			mercury_macros.h		\
> +			mercury_trace.h			\
> +			mercury_trace_alias.h		\
> +			mercury_trace_external.h 	\
> +			mercury_trace_help.h		\
> +			mercury_trace_internal.h	\
> +			mercury_trace_spy.h		\
> +			mercury_trace_tables.h
> +
> +# keep this list in alphabetical order, please
> +CFILES		= 	\
> +			mercury_trace.c			\
> +			mercury_trace_alias.c		\
> +			mercury_trace_external.c 	\
> +			mercury_trace_help.c		\
> +			mercury_trace_internal.c	\
> +			mercury_trace_spy.c		\
> +			mercury_trace_tables.c

The list of files added here doesn't match the list of files
deleted from runtime/Mmakefile, I think.

> +# Stuff for Windows DLLs
> +
> +ifeq ($(USE_DLLS),yes)
> +
> +DLL_CFLAGS	= -Dlib$(TRACE_LIB_NAME)_DEFINE_DLL
> +
> +# the following header files are created automatically by Makefile.DLLs
> +LIBMER_DLL_H	 = lib$(TRACE_LIB_NAME)_dll.h
> +LIBMER_GLOBALS_H = lib$(TRACE_LIB_NAME)_globals.h
> +
> +include $(MERCURY_DIR)/Makefile.DLLs
> +
> +else
> +
> +DLL_CFLAGS =
> +LIBMER_DLL_H =
> +LIBMER_GLOBALS_H =
> +DLL_DEF_LIB =
> +
> +endif

I think all the occurrences of `LIBMER' there should be something else.

This change will need at least one more round of reviewing.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.