[Mesa-dev] [PATCH 4/4] glsl: Change locations from yylloc to appropriate tokens positions.

[Carl Worth]

Sir Anthony <anthony at adsorbtion.org> 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 <cworth at cworth.org>

-Carl

-- 
carl.d.worth at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140205/7be8ef9e/attachment.pgp>