Re: Re: RFC: [email protected]

It would be beneficial to post this to [email protected]

Earnie.

Tony R. Ambardar wrote:
> Hi Don,
> 
> With a little break and fresh eyes I finally had a chance to look over our
> patches. They're functionally equivalent down to the CreateProcess calls,
> though mine will sometimes be conservative and generate redundant
> backslashes in the CreateProcess command-line.
> 
> So I've merged things with a eye to making it as easy as possible for the
> GNU Make maintainer to accept the patch as is, and also make it easily
> understandable. Please apply the patch to a fresh sub_proc.c and check it
> out. I've also re-run my same tests successfully. I think it's much
> cleaner and more clear than the original code.
> 
> Main changes from the original code and our first patches are:
> 
> 1. Improved and cleaned up the commentary. Some was a bit misleading.
> 
> 2. In the mem alloc loop you moved the switch " case down and relied on
> fall-through to set *enclose_in_quotes_i, but I left it where it is and
> explicitly set *enclose_in_quotes_i. I think this is clearer because there
> are really two very different reasons why we're quoting arguments: one
> case is for preserving whitespace, and the other is to work around the
> weird parsing of arguments containing "s done by MSVCRT.
> 
> 3. In the character copying loop only, use of the backslash_count variable
> and multiple while loops is needlessly complex and results in a "spread
> out" logic which can be error-prone. The equivalent logic of "running"
> backslash count can be done in just one of the else cases. This lets us
> remove all use of backslash_count and the while loop, and also get rid of
> one #if/#endif in the closing " code. The resulting logic is much clearer
> and more localized.
> 
> 
> Take care,
> Tony Ambardar
> 
> 
> ------------------------------------------------------------------------
> 
> --- mingw32-make-3.80.0-3/w32/subproc/sub_proc.c.orig	Sun Aug 31 17:58:42 2003
> +++ mingw32-make-3.80.0-3/w32/subproc/sub_proc.c	Wed Sep 17 19:18:35 2003
> @@ -968,15 +968,25 @@
>  			switch (*p) {
>  			case '\"':
>  				/*
> -				 * We have to insert a backslash for each "
> -				 * and each \ that precedes the ".
> +				 * We may have to insert a backslash or extra "
> +				 * for each " in the argument. Allocate memory
> +				 * for both cases.
>  				 */
> -				bytes_required += (backslash_count + 1);
> -				backslash_count = 0;
> +				bytes_required++;
> +				backslash_count++;
> +				/* 
> +				 * We wrap the argument in quotes to prevent errors
> +				 * due to the bizarre parsing of " in MSVCRT.
> +				 */
> +				*enclose_in_quotes_i = 1;
>  				break;
>  
>  #if !defined(HAVE_MKS_SHELL) && !defined(HAVE_CYGWIN_SHELL)
>  			case '\\':
> +				/*
> +				 * Backslashes may need to be escaped themselves
> +				 * so allocate an extra one.
> +				 */
>  				backslash_count++;
>  				break;
>  #endif
> @@ -994,16 +1004,17 @@
>  	 */
>  			case ' ':
>  			case '\t':
> +				/* Arguments with whitespace need to
> +				 * be quoted in order to parse as one. */
>  				*enclose_in_quotes_i = 1;
>  				/* fall through */
>  
>  			default:
> -				backslash_count = 0;
>  				break;
>  			}
>  
>  			/*
> -			 * Add one for each character in argv[i].
> +			 * Allocate one for each character in argv[i].
>  			 */
>  			bytes_required++;
>  
> @@ -1012,9 +1023,9 @@
>  
>  		if (*enclose_in_quotes_i) {
>  			/*
> -			 * Add one for each enclosing ",
> -			 * and one for each \ that precedes the
> -			 * closing ".
> +			 * Allocate one for each enclosing ",
> +			 * and one for each extra \ required
> +			 * within the argument.
>  			 */
>  			bytes_required += (backslash_count + 2);
>  		}
> @@ -1027,7 +1038,7 @@
>  	}
>  
>  	/*
> -	 * Add one for the terminating NULL.
> +	 * Allocate one for the terminating NULL.
>  	 */
>  	bytes_required++;
>  
> @@ -1061,35 +1072,36 @@
>  
>  	while(*argvi) {
>  		char* p = *argvi;
> -		unsigned int backslash_count = 0;
>  
>  		if (*enclose_in_quotes_i) {
> +			/*
> +			 * Add the opening ".
> +			 */
>  			*(command_line_i++) = '\"';
>  		}
>  
>  		while(*p) {
>  			if (*p == '\"') {
>  				if (cygwin_mode && have_sh) { /* HAVE_CYGWIN_SHELL */
> -					/* instead of a \", cygwin likes "" */
> +					/*
> +					 * Instead of a \", cygwin likes "".
> +					 */
>  					*(command_line_i++) = '\"';
>  				} else {
>  
> -				/*
> -				 * We have to insert a backslash for the "
> -				 * and each \ that precedes the ".
> -				 */
> -				backslash_count++;
> -
> -				while(backslash_count) {
> +					/*
> +					 * We have to insert a backslash for the ".
> +					 */
>  					*(command_line_i++) = '\\';
> -					backslash_count--;
> -				};
>  				}
>  #if !defined(HAVE_MKS_SHELL) && !defined(HAVE_CYGWIN_SHELL)
>  			} else if (*p == '\\') {
> -				backslash_count++;
> -			} else {
> -				backslash_count = 0;
> +				/*
> +				 * An original \ needs to be escaped if
> +				 * the argument is to be quoted.
> +				 */
> +				if (*enclose_in_quotes_i)
> +					*(command_line_i++) = '\\';
>  #endif
>  			}
>  
> @@ -1100,15 +1112,9 @@
>  		}
>  
>  		if (*enclose_in_quotes_i) {
> -#if !defined(HAVE_MKS_SHELL) && !defined(HAVE_CYGWIN_SHELL)
>  			/*
> -			 * Add one \ for each \ that precedes the
> -			 * closing ".
> +			 * Add the closing ".
>  			 */
> -			while(backslash_count--) {
> -				*(command_line_i++) = '\\';
> -			};
> -#endif
>  			*(command_line_i++) = '\"';
>  		}
>  

-- 
http://www.mingw.org



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf