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

[Sir Anthony]

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 <cworth at cworth.org> wrote:

> 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


-- 
Sir Anthony <anthony at adsorbtion.org>