On 06/09/2012 04:41 PM, Carl Worth wrote:
> The GLSL specification requires that #line directives be interpreted
> after macro expansion. Our existing implementation of #line macros in
> the lexer prevents conformance on this point.
>
> Moving the handling of #line from the lexer to the parser gives us the
> macro expansion we need. An additional benefit is that the
> preprocessor also now supports comments on the same line as #line
> directives.
>
> Finally, the preprocessor now emits the (fully-macro-expanded) #line
> directives into the output. This allows the full GLSL compiler to also
> see and interpret these directives so it can also generate correct
> line numbers in error messages.
> ---
> src/glsl/glcpp/glcpp-lex.l | 49
> +++++++------------------
> src/glsl/glcpp/glcpp-parse.y | 33 ++++++++++++++++-
> src/glsl/glcpp/glcpp.h | 4 ++
> src/glsl/glcpp/tests/091-hash-line.c.expected | 8 ++--
> 4 files changed, 54 insertions(+), 40 deletions(-)
Hi Carl,
Sorry for the extremely late review. (Lost this one in the inbox.)
Comments below...
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index b34f2c0..7ab58cb 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -40,12 +40,18 @@ void glcpp_set_column (int column_no , yyscan_t
> yyscanner);
>
> #define YY_NO_INPUT
>
> -#define YY_USER_ACTION \
> - do { \
> - yylloc->first_column = yycolumn + 1; \
> - yylloc->first_line = yylineno; \
> - yycolumn += yyleng; \
> - } while(0);
> +#define YY_USER_ACTION
> \
> + do { \
> + if (parser->has_new_line_number) \
> + yylineno = parser->new_line_number; \
> + if (parser->has_new_source_number) \
> + yylloc->source = parser->new_source_number; \
Could yylineno and yylloc->source be set directly from the rules, rather
than using flags and setting them here? It seems like they could, but I
imagine you tried that and it doesn't work.
> + yylloc->first_column = yycolumn + 1; \
> + yylloc->first_line = yylineno; \
> + yycolumn += yyleng; \
> + parser->has_new_line_number = 0; \
> + parser->has_new_source_number = 0; \
> + } while(0);
>
> #define YY_USER_INIT \
> do { \
> @@ -129,35 +135,8 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
> return OTHER;
> }
>
> -{HASH}line{HSPACE}+{DIGITS}{HSPACE}+{DIGITS}{HSPACE}*$ {
> - /* Eat characters until the first digit is
> - * encountered
> - */
> - char *ptr = yytext;
> - while (!isdigit(*ptr))
> - ptr++;
> -
> - /* Subtract one from the line number because
> - * yylineno is zero-based instead of
> - * one-based.
> - */
> - yylineno = strtol(ptr, &ptr, 0) - 1;
> - yylloc->source = strtol(ptr, NULL, 0);
> -}
> -
> -{HASH}line{HSPACE}+{DIGITS}{HSPACE}*$ {
> - /* Eat characters until the first digit is
> - * encountered
> - */
> - char *ptr = yytext;
> - while (!isdigit(*ptr))
> - ptr++;
> -
> - /* Subtract one from the line number because
> - * yylineno is zero-based instead of
> - * one-based.
> - */
> - yylineno = strtol(ptr, &ptr, 0) - 1;
> +{HASH}line {
> + return HASH_LINE;
> }
>
> <SKIP,INITIAL>{
> diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
> index ee00180..cc4af16 100644
> --- a/src/glsl/glcpp/glcpp-parse.y
> +++ b/src/glsl/glcpp/glcpp-parse.y
> @@ -162,7 +162,7 @@ add_builtin_define(glcpp_parser_t *parser, const char
> *name, int value);
> %lex-param {glcpp_parser_t *parser}
>
> %expect 0
> -%token COMMA_FINAL DEFINED ELIF_EXPANDED HASH HASH_DEFINE_FUNC
> HASH_DEFINE_OBJ HASH_ELIF HASH_ELSE HASH_ENDIF HASH_IF HASH_IFDEF HASH_IFNDEF
> HASH_UNDEF HASH_VERSION IDENTIFIER IF_EXPANDED INTEGER INTEGER_STRING NEWLINE
> OTHER PLACEHOLDER SPACE
> +%token COMMA_FINAL DEFINED ELIF_EXPANDED HASH HASH_DEFINE_FUNC
> HASH_DEFINE_OBJ HASH_ELIF HASH_ELSE HASH_ENDIF HASH_IF HASH_IFDEF HASH_IFNDEF
> HASH_LINE HASH_UNDEF HASH_VERSION IDENTIFIER IF_EXPANDED INTEGER
> INTEGER_STRING LINE_EXPANDED NEWLINE OTHER PLACEHOLDER SPACE
> %token PASTE
> %type <ival> expression INTEGER operator SPACE integer_constant
> %type <str> IDENTIFIER INTEGER_STRING OTHER
> @@ -208,6 +208,24 @@ expanded_line:
> | ELIF_EXPANDED expression NEWLINE {
> _glcpp_parser_skip_stack_change_if (parser, & @1, "elif", $2);
> }
> +| LINE_EXPANDED integer_constant NEWLINE {
> + parser->has_new_line_number = 1;
> + parser->new_line_number = $2;
> + ralloc_asprintf_rewrite_tail (&parser->output,
> + &parser->output_length,
> + "#line %" PRIiMAX,
> + $2);
> + }
> +| LINE_EXPANDED integer_constant integer_constant NEWLINE {
> + parser->has_new_line_number = 1;
> + parser->new_line_number = $2;
> + parser->has_new_source_number = 1;
> + parser->new_source_number = $3;
> + ralloc_asprintf_rewrite_tail (&parser->output,
> + &parser->output_length,
> + "#line %" PRIiMAX " %" PRIiMAX,
> + $2, $3);
> + }
> ;
>
> control_line:
> @@ -228,6 +246,14 @@ control_line:
> }
> ralloc_free ($2);
> }
> +| HASH_LINE pp_tokens NEWLINE {
> + if (parser->skip_stack == NULL ||
> + parser->skip_stack->type == SKIP_NO_SKIP)
> + {
> + _glcpp_parser_expand_and_lex_from (parser,
> + LINE_EXPANDED, $2);
> + }
> + }
Hm. I originally thought #line took effect regardless of any #if trees,
but it looks like that's not the case. Feeding GCC's cpp the following
yields 20, and no mention of 10:
#ifdef FOO
#line 10
#else
#line 20
#endif
So I think this is right.
> | HASH_IF conditional_tokens NEWLINE {
> /* Be careful to only evaluate the 'if' expression if
> * we are not skipping. When we are skipping, we
> @@ -1120,6 +1146,11 @@ glcpp_parser_create (const struct gl_extensions
> *extensions, int api)
> parser->info_log_length = 0;
> parser->error = 0;
>
> + parser->has_new_line_number = 0;
> + parser->new_line_number = 1;
> + parser->has_new_source_number = 0;
> + parser->new_source_number = 0;
> +
> /* Add pre-defined macros. */
> add_builtin_define(parser, "GL_ARB_draw_buffers", 1);
> add_builtin_define(parser, "GL_ARB_texture_rectangle", 1);
> diff --git a/src/glsl/glcpp/glcpp.h b/src/glsl/glcpp/glcpp.h
> index 2d7cad2..9d4005c 100644
> --- a/src/glsl/glcpp/glcpp.h
> +++ b/src/glsl/glcpp/glcpp.h
> @@ -177,6 +177,10 @@ struct glcpp_parser {
> size_t output_length;
> size_t info_log_length;
> int error;
> + int has_new_line_number;
> + int new_line_number;
> + int has_new_source_number;
> + int new_source_number;
> };
Could you please #include <stdbool.h> and use 'bool' instead of 'int'
for the has_* variables? This is not only a bit nicer to read, it
causes the compiler to generate proper warnings if you do something
silly like assign GL_FLAT to a boolean.
> struct gl_extensions;
> diff --git a/src/glsl/glcpp/tests/091-hash-line.c.expected
> b/src/glsl/glcpp/tests/091-hash-line.c.expected
> index e663398..ea29149 100644
> --- a/src/glsl/glcpp/tests/091-hash-line.c.expected
> +++ b/src/glsl/glcpp/tests/091-hash-line.c.expected
> @@ -3,11 +3,11 @@
> 1:0(1): preprocessor error: #error source 1, line 0 error
> 2:30(1): preprocessor error: #error source 2, line 30 error
>
> +#line 0
>
> +#line 25
>
> +#line 0 1
>
> -
> -
> -
> -
> +#line 30 2
Other than those minor comments, this looks good. Thanks for doing
this! Way better than my hack.
For the series:
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
