[m-dev.] For review: using alternate installation dirs, round 2

[Fergus Henderson]

On 23-Jul-1999, Warwick Harvey <wharvey at cs.monash.edu.au> wrote:
> RCS file: /home/mercury1/repository/mercury/extras/references/samples/Mmakefile,v
> retrieving revision 1.2
> diff -u -r1.2 Mmakefile
> --- Mmakefile	1999/07/20 03:39:23	1.2
> +++ Mmakefile	1999/07/23 02:46:56
> @@ -9,14 +9,8 @@
>  # We need to use a grade with trailing
>  GRADEFLAGS += --use-trail
>  
> -MGNUCFLAGS= -I..
> -
> -# Link in the reference library from ..
> -MCFLAGS += -I.. $(EXTRA_MCFLAGS)
> -MLFLAGS += -R`pwd`/.. -L.. $(EXTRA_MLFLAGS)
> -MLLIBS = -lglobal $(EXTRA_MLLIBS)
> -VPATH = ..:$(MMAKE_VPATH)
> -C2INITARGS = ../global.init
> +EXTRA_LIB_DIRS = /home/wharvey/mercury/libdirs/extras/lib/mercury
> +EXTRA_LIBRARIES = global

I know this is just a sample diff, not one that you're going to commit,
but shouldn't that be

	EXTRA_LIB_DIRS = ..

rather than using a hard-coded path name?  I think now that ml
expands relative path names there should be no need to use an
absolute path here.

> Index: scripts/mmake.in
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/scripts/mmake.in,v
> retrieving revision 1.28
> diff -u -r1.28 mmake.in
> --- mmake.in	1999/07/20 03:39:18	1.28
> +++ mmake.in	1999/07/23 02:46:57
> @@ -66,6 +66,8 @@
>  MMAKE_VARS=${MMAKE_VARS=$MMAKE_DIR/Mmake.vars}
>  MMAKE_RULES=${MMAKE_RULES=$MMAKE_DIR/Mmake.rules}
>  MERCURY_INT_DIR=${MERCURY_INT_DIR=@LIBDIR@/ints}
> +MERCURY_EXTRA_INT_DIRS=${MERCURY_EXTRA_INT_DIRS=}
> +MERCURY_EXTRA_INIT_LIB_DIRS=${MERCURY_EXTRA_INIT_LIB_DIRS=}
>  MERCURY_DEFAULT_GRADE=${MERCURY_DEFAULT_GRADE=@DEFAULT_GRADE@}
>  MKTEMP=@MKTEMP@

Adding these variable definitions here doesn't make sense, because
those definitions are not used.  Perhaps you intended to also
export them?

> Index: util/mkinit.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/util/mkinit.c,v
> retrieving revision 1.52
> diff -u -r1.52 mkinit.c
> --- mkinit.c	1999/06/29 19:08:32	1.52
> +++ mkinit.c	1999/07/23 02:46:57
> @@ -23,6 +23,8 @@
>  #include	<string.h>
>  #include	<ctype.h>
>  #include	<errno.h>
> +#include	<unistd.h>
> +#include	<sys/stat.h>

Those header files are part of POSIX, but they are not standard ANSI/ISO C,
so you should avoid using them if possible.
If not, then you really ought to use autoconf to check that they exist,
and protect the #include statements with #ifdefs, and do something
appropriate in the case when they don't exist.

In this particular case, where you were just using stat() to check the
existence of a file, the appropriate thing to do if they don't
exist is to use fopen() instead of stat().  (Of course, don't forget
to fclose() the file afterwards.)

There's an efficiency/portability/simplicity trade-off here
(choose any two).  The efficient and portable solution using #ifdef
would be best, IMHO, but is the most work.  I'd be satisfied with the
simple and portable but not maximally efficient version using fopen().
But I would rather not have the simple and efficient but not maximally
portable solution using stat().

Anyway, I think at very least it's worth abstracting the code to check
whether a file exists into a separate function.

	static bool file_exists(const char *filename);

...
> +		case 'I':
> +			tmp_slist = (String_List *)
> +					checked_malloc(sizeof(String_List));
> +			tmp_slist->next = NULL;
> +			tmp_slist->data = (char *)
> +					checked_malloc(strlen(optarg) + 1);
> +			strcpy(tmp_slist->data, optarg);
> +			*init_file_dirs_tail = tmp_slist;
> +			init_file_dirs_tail = &tmp_slist->next;
> +			break;
> +

It would be good to add a comment here saying "append the directory
name into the end of the list" or something like that.

-- 
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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------