[m-rev.] for review: fix command line quoting

[Fergus Henderson]

On 06-Aug-2002, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> Fix quoting of `--cflags', `--link-flags', etc.
> The arguments are now quoted as if each `--cflags'
> option gives a single word to be passed to the
> gcc, ml, etc. This allows directory names containing
> spaces to be passed.

Hmm.  This makes the option names a little misleading:
the names are all plural, but you can only pass one flag per option.

It's also an unnecessary annoyance in the potentially common case where
the options that you are passing do not (and cannot) contain any embedded
spaces.

And technically it breaks backwards compatibility.

Did you consider defining alternative option names `--cflag',
`--link-flag', etc., with the new semantics, and/or keeping
the old option names with their existing semantics?

> Index: compiler/compile_target_code.m
> @@ -585,10 +590,10 @@
>  	;
>  		% XXX PathSeparator should be ";" on Windows
>  		{ PathSeparator = ":" },
> -		{ join_string_list(Java_Incl_Dirs, "", "",
> +		{ join_quoted_string_list(Java_Incl_Dirs, "", "",
>  			PathSeparator, ClassPath) },
>  		{ InclOpt = string__append_list([
> -			"-classpath ", ClassPath, " "]) }
> +			"-classpath ", quote_arg(ClassPath), " "]) }

Won't that code incorrectly quote the Java_Incl_Dirs twice?

> +quote_arg(Arg0) = Arg :-
> +	ArgList = quote_arg_2(string__to_char_list(Arg0)),
> +	(
> +		ArgList = []
> +	->
> +		Arg = """"""
> +	;
> +		list__member(Char, ArgList),
> +		( char__is_whitespace(Char)
> +		; Char = ('\\')
> +		; Char = '"'
> +		)
> +	->
> +		Arg = "'" ++ string__from_char_list(ArgList) ++ "'"
> +	;
> +		Arg = string__from_char_list(ArgList)
> +	).

It's a little inconsistent to use double-quotes to quote empty
arguments but single-quotes to quote arguments containing spaces, etc.

> +:- func quote_arg_2(list(char)) = list(char).
> +
> +quote_arg_2([]) = [].
> +quote_arg_2([Char | Chars0]) = Chars :-
> +	Chars1 = quote_arg_2(Chars0),
> +	( EscapedChar = quote_char(Char) ->
> +		Chars = [('\\'), EscapedChar | Chars1]
> +	;
> +		Chars = [Char | Chars1]	
> +	).	
> +
> +
> +:- func quote_char(char) = char is semidet.
> +
> +quote_char('\\') = ('\\').
> +quote_char('"') = '"'.

Are you sure this handles arguments containing backslashes or
double quotes correctly?

It looks like this code will convert e.g. the argument foo\bar"baz into
'foo\\bar\"baz' which the Bourne shell will convert into foo\\bar\"baz
which is not the same as the original string.

Also, don't you also need to quote other shell meta-characters,
such as "$", "<", ">", "=", etc.?

I think it also won't correctly handle arguments containing
single quotes.

> Index: scripts/Mmake.vars.in
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/scripts/Mmake.vars.in,v
> retrieving revision 1.76
> diff -u -u -r1.76 Mmake.vars.in
> --- scripts/Mmake.vars.in	29 Jul 2002 07:51:07 -0000	1.76
> +++ scripts/Mmake.vars.in	6 Aug 2002 05:34:54 -0000
> @@ -159,7 +159,7 @@
>  ALL_MCGFLAGS	= $(MCGFLAGS) $(EXTRA_MCGFLAGS) $(TARGET_MCFLAGS) \
>  		  $(LIB_MCFLAGS)
>  ALL_MCSFLAGS	= $(MCSFLAGS) $(EXTRA_MCSFLAGS) $(TARGET_MCFLAGS) \
> -		  $(LIB_MCFLAGS) --cflags "$(ALL_CFLAGS)"
> +		  $(LIB_MCFLAGS) $(patsubst %,--cflags %,$(ALL_CFLAGS))

Hmm.  I'm not sure this is really going to be an improvement.
Right now it if you define

	CFLAGS = -I 'foo bar'

in your Mmakefile, then then it will get passed to mmc as

	mmc --cflags "-I 'foo bar'"

which has the intended effect.
But with your change, it would get converted into

	mmc --cflags -I --cflags 'foo --cflags bar'

which does not do the right thing.  I don't see any way the user could
work around this other than by not using Mmake.

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