[m-dev.] Using VC++ 6 with hlc.gc

[Fergus Henderson]

On 06-Jun-2000, Peter Ross <peter.ross at miscrit.be> wrote:
> Here it is!

Great -- thanks.

I have some preliminary review comments.
These are mostly just minor style issues.

> +++ boehm_gc/if_not_there.c	2000/06/06 10:10:24
> @@ -16,8 +16,7 @@
>          return(0);
>      }
>      printf("^^^^Starting command^^^^\n");
> -    execvp(argv[2], argv+2);
> -    exit(1);
> +    return execvp(argv[2], argv+2);
> 

I don't quite understand the rationale for that change.
I guess it is a work-around for some brokenness in
the MSVC implementation of execvp().
If so, that change should be inside `#ifdef _MSC_VER'.
     
> +++ compiler/mercury_compile.m	2000/06/06 10:11:41
> @@ -2696,8 +2696,9 @@
>  	    ;
>  		TraceOpt = ""
>  	    },
> -	    join_module_list(Modules, ".c", ["> ", InitCFileName], MkInitCmd0),
> -	    { string__append_list(["c2init ", TraceOpt | MkInitCmd0],
> +	    join_module_list(Modules, ".c", ["> ", InitCFileName, "'"],
> +		MkInitCmd0),
> +	    { string__append_list(["bash -c 'c2init ", TraceOpt | MkInitCmd0],
>  	    	MkInitCmd) },
>  	    invoke_system_command(MkInitCmd, MkInitOK),

It would probably be better to change `invoke_system_command', or to
add a new routine `invoke_shell_command', rather than changing the
code here.  It would also be better to avoid hard-coding this; you
could make it conditional on an option, which would be passed by
scripts/mmc, with the default value passed by scripts/mmc for that
option being determined by some autoconf variable set at configuration
time.

> Index: compiler/mlds_to_c.m
> +:- pred mlds_needs_initialization(mlds__initializer::in, bool::out) is det.
> +
> +mlds_needs_initialization(no_initializer, no).
> +mlds_needs_initialization(init_obj(_), yes).
> +mlds_needs_initialization(init_struct([]), no).
> +mlds_needs_initialization(init_struct([_|_]), yes).
> +mlds_needs_initialization(init_array(_), yes).

I suggest making that a function rather than a predicate.

> +++ scripts/Mmake.vars.in	2000/06/06 10:14:21
> @@ -179,9 +179,11 @@
>  MSPFLAGS	=
>  EXTRA_MSPFLAGS	=
>  
> -AR		= ar
> -ALL_ARFLAGS	= $(ARFLAGS) $(EXTRA_ARFLAGS) $(TARGET_ARFLAGS)
> -ARFLAGS		= cr
> +	# Note ARFLAGS at the end as MS VC++6 expects a command line of the
> +	# form: lib /OUT:libxxx.a *.o
> +AR		= @AR@
> +ALL_ARFLAGS	= $(EXTRA_ARFLAGS) $(TARGET_ARFLAGS) $(ARFLAGS)
> +ARFLAGS		= @ARFLAGS@

I think it would be clearer to introduce a different variable
called say AR_LIBFILE_OPT, similar to the OBJFILE_OPT variable
that you introduced to use instead of `-o' for the C compiler,
and to use that rather than changing the order of $(ARFLAGS)
in $(ALL_ARFLAGS) and requiring that there be no space
between $(ALL_ARFLAGS) and the library name.

> +++ trace/Mmakefile	2000/06/06 10:14:30
> @@ -124,7 +124,7 @@
>  
>  lib$(TRACE_LIB_NAME)$(DLL_DEF_LIB).a: $(OBJS)
>  	rm -f lib$(TRACE_LIB_NAME)$(DLL_DEF_LIB).a
> -	ar cr lib$(TRACE_LIB_NAME)$(DLL_DEF_LIB).a $(OBJS)
> +	$(AR) $(ARFLAGS)lib$(TRACE_LIB_NAME)$(DLL_DEF_LIB).a $(OBJS)

Here (and elsewhere) that should probably be "$(ALL_ARFLAGS)"
or perhaps "$(ALL_ARFLAGS) $(AR_LIBFILE_OPT)" rather
than "$(ARFLAGS)".

> +++ util/mkinit.c	2000/06/06 10:14:39
> @@ -23,8 +23,12 @@
...
> +#ifdef HAVE_UNISTD_H
> +#include	<unistd.h>
> +#endif

It's nicer to use indentation to indicate the nesting:

s/#include/  #include/
           ^^

> +++ runtime/mercury_memory.c	2000/06/06 10:15:35
> +++ runtime/mercury_memory_handlers.c	2000/06/06 10:15:37
> +++ runtime/mercury_memory_zones.c	2000/06/06 10:15:40
> +++ runtime/mercury_prof.c	2000/06/06 10:15:42
> +++ runtime/mercury_reg_workarounds.c	2000/06/06 10:15:42
> +++ runtime/mercury_signal.c	2000/06/06 10:15:42
> +++ runtime/mercury_timing.c	2000/06/06 10:15:43
> +++ runtime/mercury_timing.h	2000/06/06 10:15:43
> +++ runtime/mercury_trace_base.c	2000/06/06 10:15:47

Likewise.

> +++ runtime/mercury_reg_workarounds.h	2000/06/06 10:15:42
> @@ -18,6 +18,8 @@
>    #include <sys/time.h>			/* for FD_ZERO() */
>  #endif
>  
> +#include <stdlib.h>			/* needed for size_t with VC++ 6 */

I think it would be sufficient to say "for size_t".
The fact that it works without that #include on other systems
is just coincidence; it might well break on future releases
of Linux, for example.

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