[Mesa-dev] [PATCH] glsl-compiler: ast: Precise locations positions.

[Sir Anthony]

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

> Sir Anthony <anthony at adsorbtion.org> 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.worth at intel.com


-- 
Sir Anthony <anthony at adsorbtion.org>