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

[Carl Worth]

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
-------------- 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/20140204/3f7f3165/attachment.pgp>