Refactored glslang grammar files to make:...

Refactored glslang grammar files to make:
- lexer and parser reentrant
- line number handling automatic

Caveats:
- The preprocessor is still not thread-safe and full of bugs. I have another not-yet-ready patch to replace the preprocessor.
- The grammar files use options that are not supported by the old versions of flex and bison checked into compiler/tools. So I need to check-in the generated lexer-parser along with a shell script to generate them.

[Alok Priyadarshi, 2010-11-08 22:45:12]

I have added comments inline to aid quick review.

Also please look at the caveat in the CL description. If you are fine with
checking in generated lexers-parser, I will do it in the next CL.

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l
File src/compiler/glslang.l (left):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l#oldcod...
src/compiler/glslang.l:335: void PaReservedWord()
replaced by reserved_word()

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l#oldcod...
src/compiler/glslang.l:341: int PaIdentOrType(TString& id, TParseContext&
parseContextLocal, TSymbol*& symbol)
replaced by check_type()

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l#oldcod...
src/compiler/glslang.l:355: int PaParseComment(int &lineno, TParseContext&
parseContextLocal)
no longer needed. It is handled by COMMENT state.

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l
File src/compiler/glslang.l (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l#newcode69
src/compiler/glslang.l:69: "invariant"    { return(INVARIANT); }
In all actions "pyylval->lex.line = yylineno" has been replaced by
YY_USER_ACTION

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.y
File src/compiler/glslang.y (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.y#newcode23
src/compiler/glslang.y:23: %pure-parser
This file mostly renames parseContext to context

[kbr1, 2010-11-09 00:05:15]

I didn't go through this in great detail but overall it looks like a very good
refactoring and cleanup. Two minor issues.

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l
File src/compiler/glslang.l (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l#newcode5
src/compiler/glslang.l:5: /*
This re-beginning of comment looks wrong (even though it's harmless).

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.y
File src/compiler/glslang.y (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.y#newcode5
src/compiler/glslang.y:5: /*
Same note about restart of comment.

[dgkoch, 2010-11-09 14:20:32]

We had looked into using a newer version of flex and bison earlier in the
project, but the licenses on the newer windows versions make it so that we can't
redistribute them directly in the project.

If we are going to make the grammar so that it no longer works with the old
bison & flex in compiler/tools we need to do the following:

One of the following to avoid breaking the stand-alone .vcproj files:
1 a) Add an external dependency on having Flex & Bison installed, update the
vcproj pre-build steps appropriately to use these, *or*
   b) check in the generated files, and add a script or optional build-step to
re-generate these if Flex & Bison are installed.
2) remove the obsolete tools
3) update the build documentation on the wiki.

Daniel

http://codereview.appspot.com/2992041/diff/2001/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/ParseHelper.cpp#...
src/compiler/ParseHelper.cpp:20: }
shouldn't these be in a header (ParseHelper.h?), or are you trying to keep them
more private?

http://codereview.appspot.com/2992041/diff/2001/src/compiler/ParseHelper.cpp#...
src/compiler/ParseHelper.cpp:231: ++numErrors;
is this is a warning, should we be increasing the error count?

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l
File src/compiler/glslang.l (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l#newcode69
src/compiler/glslang.l:69: "invariant"    { return(INVARIANT); }
On 2010/11/08 22:45:12, Alok Priyadarshi wrote:
> In all actions "pyylval->lex.line = yylineno" has been replaced by
> YY_USER_ACTION

is YY_USER_ACTION something that is implicitly invoked?

[Alok Priyadarshi, 2010-11-09 16:33:47]

I will check-in the generated lexer-parser files along with the shell script. I
will also update the wiki.

http://codereview.appspot.com/2992041/diff/2001/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/ParseHelper.cpp#...
src/compiler/ParseHelper.cpp:20: }
On 2010/11/09 14:20:32, dgkoch wrote:
> shouldn't these be in a header (ParseHelper.h?), or are you trying to keep
them
> more private?

They need not be in the header file. They are just static helper functions used
in this file only.

http://codereview.appspot.com/2992041/diff/2001/src/compiler/ParseHelper.cpp#...
src/compiler/ParseHelper.cpp:231: ++numErrors;
On 2010/11/09 14:20:32, dgkoch wrote:
> is this is a warning, should we be increasing the error count?

Good catch - It should not increase error count.

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l
File src/compiler/glslang.l (right):

http://codereview.appspot.com/2992041/diff/2001/src/compiler/glslang.l#newcode69
src/compiler/glslang.l:69: "invariant"    { return(INVARIANT); }
Yes.

[Alok Priyadarshi, 2010-11-09 19:50:45]

All DONE:
- Added the generated files along with shell script.
- Removed old flex and bison tools.

I will update the wiki shortly.

[Alok Priyadarshi, 2010-11-11 17:18:04]

Hi Daniel:
Do you want to take a final look before I commit?

Thanks,
-Alok

[dgkoch, 2010-11-12 14:53:05]

Sorry for the delay.  Was OOO a couple of days this week.

No issues detected.  
LGTM provided the CTS still passes without problems.

Daniel

[Alok Priyadarshi, 2010-11-12 15:42:39]

> LGTM provided the CTS still passes without problems.

Yes CTS still passes. Committing...DONE!