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

[Ian Romanick]

On 11/30/2012 02:29 PM, Kenneth Graunke wrote:
> 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.

And GLES3. :)

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

I think this should, at the very least, be the default behavior.  We 
know this will break Savage2.  The alternate behavior (to enable via 
driconf) should be either ignore line continuation altogether or ignore 
line continuation in comments.  The former will be easier, but I'm not 
sure if it will cause other problems in that game.

>> 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.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>