[Mesa-dev] [PATCH] glsl-compiler: ast: Precise locations positions.
I'm using mesa glsl-compiler as backend for glsl-debugger interface with heavy use of locations for graphical representation. Locations proposed by original parser is wrong in most cases, because it uses yylloc for all bison rules. This patch includes: 1. Change locations setup in glsl_parser.yy from yylloc to appropriate token locations. 2. Addition of two fields in ast_node location to hold end position of token. 3. Addition of ast_node method to setup range locations (for aggregate tokens). 4. Fix for glcpp-lex.l. It handled spaces wrong and convert two adjacent spaces into one, which added location offset for shaders with indentation. --- src/glsl/ast.h | 36 +-- src/glsl/glcpp/glcpp-lex.l | 5 +- src/glsl/glsl_lexer.ll | 3 +- src/glsl/glsl_parser.yy | 215 +--- src/glsl/glsl_parser_extras.cpp | 6 +- 5 files changed, 150 insertions(+), 115 deletions(-) -- 1.8.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl-compiler: ast: Precise locations positions.
1. Change locations setup in glsl_parser.yy from yylloc to appropriate token
locations.
2. Addition of two fields in ast_node location to hold end position of token.
3. Addition of ast_node method to setup range locations (for aggregate tokens).
4. Fix for glcpp-lex.l. It handled spaces wrong and convert two adjacent spaces
into one, which added location offset for shaders with indentation.
---
src/glsl/ast.h | 36 +--
src/glsl/glcpp/glcpp-lex.l | 5 +-
src/glsl/glsl_lexer.ll | 3 +-
src/glsl/glsl_parser.yy | 215 +---
src/glsl/glsl_parser_extras.cpp | 6 +-
5 files changed, 150 insertions(+), 115 deletions(-)
diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index 0bda28d..9b5bc47 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -75,10 +75,10 @@ public:
struct YYLTYPE locp;
locp.source = this->location.source;
- locp.first_line = this->location.line;
- locp.first_column = this->location.column;
- locp.last_line = locp.first_line;
- locp.last_column = locp.first_column;
+ locp.first_line = this->location.first_line;
+ locp.first_column = this->location.first_column;
+ locp.last_line = this->location.last_line;
+ locp.last_column = this->location.last_column;
return locp;
}
@@ -91,17 +91,35 @@ public:
void set_location(const struct YYLTYPE &locp)
{
this->location.source = locp.source;
- this->location.line = locp.first_line;
- this->location.column = locp.first_column;
+ this->location.first_line = locp.first_line;
+ this->location.first_column = locp.first_column;
+ this->location.last_line = locp.last_line;
+ this->location.last_column = locp.last_column;
+ }
+
+ /**
+* Set the source location range of an AST node using two location nodes
+*
+* \sa ast_node::set_location
+*/
+ void set_location_range(const struct YYLTYPE &begin, const struct YYLTYPE
&end)
+ {
+ this->location.source = begin.source;
+ this->location.first_line = begin.first_line;
+ this->location.last_line = end.last_line;
+ this->location.first_column = begin.first_column;
+ this->location.last_column = end.last_column;
}
/**
* Source location of the AST node.
*/
struct {
- unsigned source;/**< GLSL source number. */
- unsigned line; /**< Line number within the source string. */
- unsigned column;/**< Column in the line. */
+ unsigned source; /**< GLSL source number. */
+ unsigned first_line; /**< Line number within the source string. */
+ unsigned first_column;/**< Column in the line. */
+ unsigned last_line; /**< Line number within the source string. */
+ unsigned last_column; /**< Column in the line. */
} location;
exec_node link;
diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
index ea3b862..188e454 100644
--- a/src/glsl/glcpp/glcpp-lex.l
+++ b/src/glsl/glcpp/glcpp-lex.l
@@ -47,8 +47,9 @@ void glcpp_set_column (int column_no , yyscan_t yyscanner);
if (parser->has_new_source_number) \
yylloc->source = parser->new_source_number; \
yylloc->first_column = yycolumn + 1;\
- yylloc->first_line = yylineno; \
+ yylloc->first_line = yylloc->last_line = yylineno; \
yycolumn += yyleng; \
+ yylloc->last_column = yycolumn + 1; \
parser->has_new_line_number = 0;\
parser->has_new_source_number = 0; \
} while(0);
@@ -337,7 +338,7 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
return OTHER;
}
-{HSPACE}+ {
+{HSPACE} {
if (yyextra->space_tokens) {
return SPACE;
}
diff --git a/src/glsl/glsl_lexer.ll b/src/glsl/glsl_lexer.ll
index 50875bf..e7766a8 100644
--- a/src/glsl/glsl_lexer.ll
+++ b/src/glsl/glsl_lexer.ll
@@ -38,8 +38,9 @@ static int classify_identifier(struct _mesa_glsl_parse_state
*, const char *);
do {\
yylloc->source = 0; \
yylloc->first_column = yycolumn + 1; \
- yylloc->first_line = yylineno + 1; \
+ yylloc->first_line = yylloc->last_line = yylineno + 1; \
yycolumn += yyleng; \
+ yylloc->last_column = yycolumn + 1; \
} while(0);
#define YY_USER_INIT yylineno = 0; yycolumn = 0;
diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
index 928c57e..c6a585f 100644
--- a/src/glsl/glsl_parser.yy
+++ b/src/glsl/glsl_parser.yy
@@ -386,35 +386,35 @@ primary_e
[Mesa-dev] [PATCH 4/4] glsl: Change locations from yylloc to appropriate tokens positions.
---
src/glsl/glsl_parser.yy | 215 +---
1 file changed, 114 insertions(+), 101 deletions(-)
diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
index 928c57e..c6a585f 100644
--- a/src/glsl/glsl_parser.yy
+++ b/src/glsl/glsl_parser.yy
@@ -386,35 +386,35 @@ primary_expression:
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_identifier, NULL, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->primary_expression.identifier = $1;
}
| INTCONSTANT
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_int_constant, NULL, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->primary_expression.int_constant = $1;
}
| UINTCONSTANT
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_uint_constant, NULL, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->primary_expression.uint_constant = $1;
}
| FLOATCONSTANT
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_float_constant, NULL, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->primary_expression.float_constant = $1;
}
| BOOLCONSTANT
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_bool_constant, NULL, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->primary_expression.bool_constant = $1;
}
| '(' expression ')'
@@ -429,7 +429,7 @@ postfix_expression:
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_array_index, $1, $3, NULL);
- $$->set_location(yylloc);
+ $$->set_location_range(@1, @4);
}
| function_call
{
@@ -439,20 +439,20 @@ postfix_expression:
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_field_selection, $1, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location_range(@1, @3);
$$->primary_expression.identifier = $3;
}
| postfix_expression INC_OP
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_post_inc, $1, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location_range(@1, @2);
}
| postfix_expression DEC_OP
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_post_dec, $1, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location_range(@1, @2);
}
;
@@ -470,7 +470,7 @@ function_call_or_method:
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_field_selection, $1, $3, NULL);
- $$->set_location(yylloc);
+ $$->set_location_range(@1, @3);
}
;
@@ -488,13 +488,13 @@ function_call_header_with_parameters:
function_call_header assignment_expression
{
$$ = $1;
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->expressions.push_tail(& $2->link);
}
| function_call_header_with_parameters ',' assignment_expression
{
$$ = $1;
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->expressions.push_tail(& $3->link);
}
;
@@ -511,21 +511,23 @@ function_identifier:
{
void *ctx = state;
$$ = new(ctx) ast_function_expression($1);
- $$->set_location(yylloc);
+ $$->set_location(@1);
}
| variable_identifier
{
void *ctx = state;
ast_expression *callee = new(ctx) ast_expression($1);
+ callee->set_location(@1);
$$ = new(ctx) ast_function_expression(callee);
- $$->set_location(yylloc);
+ $$->set_location(@1);
}
| FIELD_SELECTION
{
void *ctx = state;
ast_expression *callee = new(ctx) ast_expression($1);
+ callee->set_location(@1);
$$ = new(ctx) ast_function_expression(callee);
- $$->set_location(yylloc);
+ $$->set_location(@1);
}
;
@@ -543,13 +545,13 @@ method_call_header_with_parameters:
method_call_header assignment_expression
{
$$ = $1;
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->expressions.push_tail(& $2->link);
}
| method_call_header_with_parameters ',' assignment_expression
{
$$ = $1;
- $$->set_location(yylloc);
+ $$->set_location(@1);
$$->expressions.push_tail(& $3->link);
}
;
@@ -562,8 +564,9 @@ method_call_header:
{
void *ctx = state;
ast_expression *callee = new(ctx) ast_expression($1);
+ callee->set_location(@1);
$$ = new(ctx) ast_function_expression(callee);
- $$->set_location(yylloc);
+ $$->set_location(@1);
}
;
@@ -574,19 +577,19 @@ unary_expression:
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_pre_inc, $2, NULL, NULL);
- $$->set_location(yylloc);
+ $$->set_location(@1);
}
| DEC_OP unary_expression
{
void *ctx = state;
$$ = new(ctx) ast_expression(ast_pre_dec, $2,
[Mesa-dev] [PATCH 0/4] glsl: Precise location positions for ast_node.
These patches changes the ast_node location holding by adding tokens end position and assignment of it along with previously tracked start position. Bison rules was updated to set appropriate tokens locations instead of yylloc. Sir Anthony (4): glsl: Update lexers in glsl and glcpp to hande end position of token. glsl: Extend ast location structure to hande end token position. glsl: Add ast_node method to set location range. glsl: Change locations from yylloc to appropriate tokens positions. src/glsl/ast.h | 36 +-- src/glsl/glcpp/glcpp-lex.l | 3 +- src/glsl/glsl_lexer.ll | 3 +- src/glsl/glsl_parser.yy | 215 +--- src/glsl/glsl_parser_extras.cpp | 6 +- 5 files changed, 149 insertions(+), 114 deletions(-) -- 1.8.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glcpp: Do not remove spaces to preserve locations.
After preprocessing by glcpp all adjacent spaces were replaced by
single one and glsl parser received column-shifted shader source.
It negatively affected ast location set up and produced wrong error
messages for heavily-spaced shaders.
---
src/glsl/glcpp/glcpp-lex.l | 2 +-
src/glsl/glcpp/tests/000-content-with-spaces.c | 2 +-
src/glsl/glcpp/tests/000-content-with-spaces.c.expected | 2 +-
src/glsl/glcpp/tests/100-macro-with-colon.c.expected| 4 ++--
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
index ea3b862..c0709a2 100644
--- a/src/glsl/glcpp/glcpp-lex.l
+++ b/src/glsl/glcpp/glcpp-lex.l
@@ -337,7 +337,7 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
return OTHER;
}
-{HSPACE}+ {
+{HSPACE} {
if (yyextra->space_tokens) {
return SPACE;
}
diff --git a/src/glsl/glcpp/tests/000-content-with-spaces.c
b/src/glsl/glcpp/tests/000-content-with-spaces.c
index 696cb3a..1f2320e 100644
--- a/src/glsl/glcpp/tests/000-content-with-spaces.c
+++ b/src/glsl/glcpp/tests/000-content-with-spaces.c
@@ -1 +1 @@
-this is four tokens
+ this is four tokens with spaces
diff --git a/src/glsl/glcpp/tests/000-content-with-spaces.c.expected
b/src/glsl/glcpp/tests/000-content-with-spaces.c.expected
index 83f7834..5e17ec9 100644
--- a/src/glsl/glcpp/tests/000-content-with-spaces.c.expected
+++ b/src/glsl/glcpp/tests/000-content-with-spaces.c.expected
@@ -1,2 +1,2 @@
-this is four tokens
+ this is four tokens with spaces
diff --git a/src/glsl/glcpp/tests/100-macro-with-colon.c.expected
b/src/glsl/glcpp/tests/100-macro-with-colon.c.expected
index 6cfac25..36f98aa 100644
--- a/src/glsl/glcpp/tests/100-macro-with-colon.c.expected
+++ b/src/glsl/glcpp/tests/100-macro-with-colon.c.expected
@@ -2,7 +2,7 @@
switch (1) {
- case 1 + 2:
- break;
+ case 1 + 2:
+ break;
}
--
1.8.3.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/4] glsl: Update lexers in glsl and glcpp to hande end position of token.
---
src/glsl/glcpp/glcpp-lex.l | 3 ++-
src/glsl/glsl_lexer.ll | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
index c0709a2..188e454 100644
--- a/src/glsl/glcpp/glcpp-lex.l
+++ b/src/glsl/glcpp/glcpp-lex.l
@@ -47,8 +47,9 @@ void glcpp_set_column (int column_no , yyscan_t yyscanner);
if (parser->has_new_source_number) \
yylloc->source = parser->new_source_number; \
yylloc->first_column = yycolumn + 1;\
- yylloc->first_line = yylineno; \
+ yylloc->first_line = yylloc->last_line = yylineno; \
yycolumn += yyleng; \
+ yylloc->last_column = yycolumn + 1; \
parser->has_new_line_number = 0;\
parser->has_new_source_number = 0; \
} while(0);
diff --git a/src/glsl/glsl_lexer.ll b/src/glsl/glsl_lexer.ll
index 50875bf..e7766a8 100644
--- a/src/glsl/glsl_lexer.ll
+++ b/src/glsl/glsl_lexer.ll
@@ -38,8 +38,9 @@ static int classify_identifier(struct _mesa_glsl_parse_state
*, const char *);
do {\
yylloc->source = 0; \
yylloc->first_column = yycolumn + 1; \
- yylloc->first_line = yylineno + 1; \
+ yylloc->first_line = yylloc->last_line = yylineno + 1; \
yycolumn += yyleng; \
+ yylloc->last_column = yycolumn + 1; \
} while(0);
#define YY_USER_INIT yylineno = 0; yycolumn = 0;
--
1.8.3.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/4] glsl: Add ast_node method to set location range.
---
src/glsl/ast.h | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index a9352f9..8e54ddc 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -98,6 +98,20 @@ public:
}
/**
+* Set the source location range of an AST node using two location nodes
+*
+* \sa ast_node::set_location
+*/
+ void set_location_range(const struct YYLTYPE &begin, const struct YYLTYPE
&end)
+ {
+ this->location.source = begin.source;
+ this->location.first_line = begin.first_line;
+ this->location.last_line = end.last_line;
+ this->location.first_column = begin.first_column;
+ this->location.last_column = end.last_column;
+ }
+
+ /**
* Source location of the AST node.
*/
struct {
--
1.8.3.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/4] glsl: Extend ast location structure to hande end token position.
---
src/glsl/ast.h | 22 +-
src/glsl/glsl_parser_extras.cpp | 6 --
2 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index 2d6f3a2..a9352f9 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -75,10 +75,10 @@ public:
struct YYLTYPE locp;
locp.source = this->location.source;
- locp.first_line = this->location.line;
- locp.first_column = this->location.column;
- locp.last_line = locp.first_line;
- locp.last_column = locp.first_column;
+ locp.first_line = this->location.first_line;
+ locp.first_column = this->location.first_column;
+ locp.last_line = this->location.last_line;
+ locp.last_column = this->location.last_column;
return locp;
}
@@ -91,17 +91,21 @@ public:
void set_location(const struct YYLTYPE &locp)
{
this->location.source = locp.source;
- this->location.line = locp.first_line;
- this->location.column = locp.first_column;
+ this->location.first_line = locp.first_line;
+ this->location.first_column = locp.first_column;
+ this->location.last_line = locp.last_line;
+ this->location.last_column = locp.last_column;
}
/**
* Source location of the AST node.
*/
struct {
- unsigned source;/**< GLSL source number. */
- unsigned line; /**< Line number within the source string. */
- unsigned column;/**< Column in the line. */
+ unsigned source; /**< GLSL source number. */
+ unsigned first_line; /**< Line number within the source string. */
+ unsigned first_column;/**< Column in the line. */
+ unsigned last_line; /**< Line number within the source string. */
+ unsigned last_column; /**< Column in the line. */
} location;
exec_node link;
diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
index 87784ed..7dfae8d 100644
--- a/src/glsl/glsl_parser_extras.cpp
+++ b/src/glsl/glsl_parser_extras.cpp
@@ -788,8 +788,10 @@ ast_node::print(void) const
ast_node::ast_node(void)
{
this->location.source = 0;
- this->location.line = 0;
- this->location.column = 0;
+ this->location.first_line = 0;
+ this->location.first_column = 0;
+ this->location.last_line = 0;
+ this->location.last_column = 0;
}
--
1.8.3.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl-compiler: ast: Precise locations positions.
Thanks for you response. First three changes pursued one purpose and meaningless without each other, but glcpp changes can be separated from it. I think, glsl_parser.yy may be separated too, because it have a lot of alteration. I'll do it today. > Or did you mean just that it computed location incorrectly in the > case of collapsing adjacent spaces? Exactly, glsl_parser receives source after glcpp strip additional spaces from it. This have no point for next parsing, because parser ignores all spaces anyway, but it has negative effect on location determination. As for tests, it means 000-content-with-spaces must work the opposite way. I'll change it and add to glcpp patch too. On Tue, 04 Feb 2014 16:06:46 -0800 Carl Worth wrote: > Sir Anthony writes: > > 1. Change locations setup in glsl_parser.yy from yylloc to appropriate > > token locations. > > 2. Addition of two fields in ast_node location to hold end position of > > token. > > 3. Addition of ast_node method to setup range locations (for aggregate > > tokens). > > 4. Fix for glcpp-lex.l. It handled spaces wrong and convert two > > adjacent spaces into one, which added location offset for shaders with > > indentation. > > Hello, Sir. > > Thanks for contributing your patch! > > You've described 4 different changes in a single patch. > > Could you please split this up into 2 or more patches? Ideally each > patch would include a single change, (and perhaps depend on previous > changes). > > This would make the patches easier to review, and improve our ability to > analyze the commit history in the future, (such as with "git bisect"). > > For my part, I haven't done much in the glsl_lexer.ll nor glsl_parser.yy > files, but I'm one of the primary authors of the glcpp-lex.l code. So I > would like to review the changes there independently from the rest of > the patch. > > And on that point, I'm a bit confused by the patch to glcpp-lex.l Your > commit message says it "handled spaces wrong and convert two adjacent > spaces into one" so I expected to see the patch changing something about > the conversion of adjacent spaces, (but I don't see anything like > that). Or did you mean just that it computed location incorrectly in the > case of collapsing adjacent spaces? > > Also, for glcpp, we have a collection of small tests in the glcpp/tests > directory. These tests are invoked by "make check". If you are changing > the behavior of glcpp, then you should be updating any tests where the > desired output is changed. And if you are fixing a bug, but no existing > test changes its output, then we need to be adding a new test to cover > that case. > > Thanks again, > > -Carl > > -- > carl.d.wo...@intel.com -- Sir Anthony ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: Make ast_node location comments more informative.
---
src/glsl/ast.h | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index 8e54ddc..50b95bf 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -116,10 +116,10 @@ public:
*/
struct {
unsigned source; /**< GLSL source number. */
- unsigned first_line; /**< Line number within the source string. */
- unsigned first_column;/**< Column in the line. */
- unsigned last_line; /**< Line number within the source string. */
- unsigned last_column; /**< Column in the line. */
+ unsigned first_line; /**< First line number within the source
string. */
+ unsigned first_column;/**< First column in the first line. */
+ unsigned last_line; /**< Last line number within the source
string. */
+ unsigned last_column; /**< Last column in the last line. */
} location;
exec_node link;
--
1.8.3.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] glsl: Change locations from yylloc to appropriate tokens positions.
This is tough question, I thought about it some time and concluded that some
function class must include body range, otherwise there will be no easy way to
get function end (it contents may be iterated (I did it in my code before I
realized I need to change locations placing itself, it was ugly), but then end
will point at last element end, not actual body end, which may be separated far
away by spaces and newlines). Function header already have range for name, if
it required, and only reasonable place to do it is here. I think additional
check needed for what lexical errors really may occurs here to throw message if
any. Anything I can think of now is prototype errors either components with
their own locations.
On Wed, 05 Feb 2014 12:41:58 -0800
Carl Worth wrote:
> Sir Anthony writes:
>
> Thanks for this patch series. It looks great to me. I sent a separate
> email with a small comment about some comments. The only other review
> comment I have is:
>
> > @@ -2127,7 +2138,7 @@ function_definition:
> > {
> >void *ctx = state;
> >$$ = new(ctx) ast_function_definition();
> > - $$->set_location(yylloc);
> > + $$->set_location_range(@1, @2);
> >$$->prototype = $1;
> >$$->body = $2;
>
> Doesn't this set the range for this definition to the entire range of
> the function (declaration and body)? In one sense, that's a correct
> range for what is being parsed here. But on the other hand, would this
> actually be a useful range to emit in an error message? Or would it be
> better to just direct the error message to the location of the prototype
> itself?
>
> I don't have strong feelings one way or the other, but just wanted to
> raise this issue to see if this is what you actually intended in this
> case.
>
> Regardless of whether you restrict the range in this case, the entire
> series is:
>
> Reviewed-by: Carl Worth
>
> -Carl
>
> --
> carl.d.wo...@intel.com
--
Sir Anthony
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] glsl: Change locations from yylloc to appropriate tokens positions.
Yes, I will be very grateful if someone will do it. Thanks. On Thu, 06 Feb 2014 12:40:21 -0800 Carl Worth wrote: > Sir Anthony writes: > > This is tough question, I thought about it some time and concluded > > that some function class must include body range, otherwise there will > > be no easy way to get function end > > Good. That's all I wanted to hear, that you had intentionally thought > about this case. And your conclusion seems reasonable to me. So, thanks. > > I assume you don't have commit access so you'll need someone else to > push your patches? > > -Carl > > -- > carl.d.wo...@intel.com -- Sir Anthony ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
