[Mesa-dev] [PATCH] glcpp: Rewrite line-continuation support to act globally.

[Kenneth Graunke]

On 11/29/2012 03:44 PM, Carl Worth wrote:
> Previously, we were only supporting line-continuation backslash characters
> within lines of pre-processor directives, (as per the specification). With
> OpenGL 4.3, line continuations are now supported anywhere within a shader.

OpenGL 4.2, actually.

I'm also not sure if we decided we can get away with changing this 
behavior.  I'd really like to, as the new rules are much more sensible, 
consistent, and efficient...

> While changing this, also fix a bug where the preprocessor was ignoring line
> continuation characters when a line ended in multiple backslash characters.
>
> The new code is also more efficient than the old. Previously, we would perform
> a ralloc copy at each newline. We now perform copies only at each occurrence
> of a line-continuation.
> ---
>   src/glsl/glcpp/pp.c |  108 ++++++++++++++++++++-------------------------------
>   1 file changed, 42 insertions(+), 66 deletions(-)
>
> diff --git a/src/glsl/glcpp/pp.c b/src/glsl/glcpp/pp.c
> index 11b2941..1135570 100644
> --- a/src/glsl/glcpp/pp.c
> +++ b/src/glsl/glcpp/pp.c
> @@ -70,82 +70,58 @@ glcpp_warning (YYLTYPE *locp, glcpp_parser_t *parser, const char *fmt, ...)
>   				     &parser->info_log_length, "\n");
>   }
>
> -/* Searches backwards for '^ *#' from a given starting point. */
> -static int
> -in_directive(const char *shader, const char *ptr)
> -{
> -	assert(ptr >= shader);
> -
> -	/* Search backwards for '#'. If we find a \n first, it doesn't count */
> -	for (; ptr >= shader && *ptr != '#'; ptr--) {
> -		if (*ptr == '\n')
> -			return 0;
> -	}
> -	if (ptr >= shader) {
> -		/* Found '#'...look for spaces preceded by a newline */
> -		for (ptr--; ptr >= shader && isblank(*ptr); ptr--);
> -		// FIXME: I don't think the '\n' case can happen
> -		if (ptr < shader || *ptr == '\n')
> -			return 1;
> -	}
> -	return 0;
> -}
> -
> -/* Remove any line continuation characters in preprocessing directives.
> - * However, ignore any in GLSL code, as "There is no line continuation
> - * character" (1.30 page 9) in GLSL.
> +/* Remove any line continuation characters in the shader, (whether in
> + * preprocessing directives or in GLSL code).
>    */
>   static char *
>   remove_line_continuations(glcpp_parser_t *ctx, const char *shader)
>   {
> -	int in_continued_line = 0;
> -	int extra_newlines = 0;
>   	char *clean = ralloc_strdup(ctx, "");
> -	const char *search_start = shader;
> -	const char *newline;
> -	while ((newline = strchr(search_start, '\n')) != NULL) {
> -		const char *backslash = NULL;
> -
> -		/* # of characters preceding the newline. */
> -		int n = newline - shader;
> -
> -		/* Find the preceding '\', if it exists */
> -		if (n >= 1 && newline[-1] == '\\')
> -			backslash = newline - 1;
> -		else if (n >= 2 && newline[-1] == '\r' && newline[-2] == '\\')
> -			backslash = newline - 2;
> -
> -		/* Double backslashes don't count (the backslash is escaped) */
> -		if (backslash != NULL && backslash[-1] == '\\') {
> -			backslash = NULL;
> -		}
> -
> -		if (backslash != NULL) {
> -			/* We found a line continuation, but do we care? */
> -			if (!in_continued_line) {
> -				if (in_directive(shader, backslash)) {
> -					in_continued_line = 1;
> -					extra_newlines = 0;
> -				}
> -			}
> -			if (in_continued_line) {
> -				/* Copy everything before the \ */
> -				ralloc_strncat(&clean, shader, backslash - shader);
> +	const char *backslash, *newline;
> +	int collapsed_newlines = 0;
> +
> +	while (1) {

while (true) ?

> +		backslash = strchr(shader, '\\');
> +
> +		/* If we have previously collapsed any
> +		 * line-continuations, then we want to insert
> +		 * additional newlines at the next occurrence of a
> +		 * newline character to avoid changing any line
> +		 * numbers. */

Please move the */ to its own line.  I'd also wrap the lines later 
(78-ish?), but that's just me...

> +		if (collapsed_newlines) {
> +			newline = strchr(shader, '\n');
> +			if (newline &&
> +			    (backslash == NULL || newline < backslash))
> +			{
> +				ralloc_strncat(&clean, shader,
> +					       newline - shader + 1);
> +				while (collapsed_newlines--)
> +					ralloc_strcat(&clean, "\n");
>   				shader = newline + 1;
> -				extra_newlines++;
>   			}
> -		} else if (in_continued_line) {
> -			/* Copy everything up to and including the \n */
> -			ralloc_strncat(&clean, shader, newline - shader + 1);
> -			shader = newline + 1;
> -			/* Output extra newlines to make line numbers match */
> -			for (; extra_newlines > 0; extra_newlines--)
> -				ralloc_strcat(&clean, "\n");
> -			in_continued_line = 0;
>   		}
> -		search_start = newline + 1;
> +
> +		if (backslash == NULL)
> +			break;
> +
> +		/* At each line continuation, (backslash followed by a
> +		 * newline), copy all preceding text to the output,
> +		 * then advance the shader pointer to the character
> +		 * after the newline. */

Please un-cuddle the comments here.

> +		if (backslash[1] == '\n' ||
> +		    (backslash[1] == '\r' && backslash[2] == '\n'))
> +		{
> +			collapsed_newlines++;
> +			ralloc_strncat(&clean, shader, backslash - shader);
> +			if (backslash[1] == '\n')
> +				shader = backslash + 2;
> +			else
> +				shader = backslash + 3;
> +		}
>   	}
> +
>   	ralloc_strcat(&clean, shader);
> +
>   	return clean;
>   }

Other than than, looks good to me...thanks Carl!

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Someday we may want to use ralloc's rewrite_tail functions to avoid the 
extra strlen() overhead.  But that can be done separately.