Changes to handle comments properly and associated tests.

Changes to handle comments properly and associated tests.
Committed: https://code.google.com/p/angleproject/source/detail?r=1059

[Alok Priyadarshi, 2012-04-26 05:11:15]

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp.l
File src/compiler/preprocessor/new/pp.l (right):

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp...
src/compiler/preprocessor/new/pp.l:68: /* Line breaks are just counted - not
returned. */
GLSL spec says "New-lines are not eliminated by comments". I am not sure exactly
what it means. If we keep the newlines inside comments for further processing
then this is invalid:

# /* foo
*/ define bar 1

Here if we ignore the newline after foo then this is equivalent to "# define bar
1", otherwise it is invalid. gcc and VC++ consider it valid. For now I have
matched their behavior by just counting but removing them from further
processing.

[kbr1, 2012-04-26 22:03:16]

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp.l
File src/compiler/preprocessor/new/pp.l (right):

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp...
src/compiler/preprocessor/new/pp.l:68: /* Line breaks are just counted - not
returned. */
On 2012/04/26 05:11:15, Alok Priyadarshi wrote:
> GLSL spec says "New-lines are not eliminated by comments". I am not sure
exactly
> what it means. If we keep the newlines inside comments for further processing
> then this is invalid:
> 
> # /* foo
> */ define bar 1
> 
> Here if we ignore the newline after foo then this is equivalent to "# define
bar
> 1", otherwise it is invalid. gcc and VC++ consider it valid. For now I have
> matched their behavior by just counting but removing them from further
> processing.

The reason for the wording in the GLSL spec is almost certainly to avoid having
multi-line comments mess up the line numbers reported in compilation errors.

If the preprocessor only counts line numbers but doesn't transmit them to the
later parsing phase (for example, via #line directives) then any validation
errors will report incorrect line numbers.

I would not go to great lengths to make your use case of #defines split by
comments work correctly. It is absolutely essential, though, that the line
numbers match up all the way through the shader validation and translation.

http://codereview.appspot.com/6111059/diff/1/tests/preprocessor_tests/comment...
File tests/preprocessor_tests/comment_test.cpp (right):

http://codereview.appspot.com/6111059/diff/1/tests/preprocessor_tests/comment...
tests/preprocessor_tests/comment_test.cpp:61: }
Need tests ensuring that multi-line comments followed by errors report the
correct line number for the error.

[Alok Priyadarshi, 2012-04-26 22:38:39]

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp.l
File src/compiler/preprocessor/new/pp.l (right):

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp...
src/compiler/preprocessor/new/pp.l:68: /* Line breaks are just counted - not
returned. */
On 2012/04/26 22:03:16, kbr1 wrote:
> On 2012/04/26 05:11:15, Alok Priyadarshi wrote:
> > GLSL spec says "New-lines are not eliminated by comments". I am not sure
> exactly
> > what it means. If we keep the newlines inside comments for further
processing
> > then this is invalid:
> > 
> > # /* foo
> > */ define bar 1
> > 
> > Here if we ignore the newline after foo then this is equivalent to "# define
> bar
> > 1", otherwise it is invalid. gcc and VC++ consider it valid. For now I have
> > matched their behavior by just counting but removing them from further
> > processing.
> 
> The reason for the wording in the GLSL spec is almost certainly to avoid
having
> multi-line comments mess up the line numbers reported in compilation errors.
> 
> If the preprocessor only counts line numbers but doesn't transmit them to the
> later parsing phase (for example, via #line directives) then any validation
> errors will report incorrect line numbers.
> 
> I would not go to great lengths to make your use case of #defines split by
> comments work correctly. It is absolutely essential, though, that the line
> numbers match up all the way through the shader validation and translation.

What I have right now should not mess up the line numbers since we are counting
the line numbers at a common location. Even #line directive will update the same
yylineno variable in the lexer.

So if I make sure that line number reporting is correct, what I have right now
is correct?

http://codereview.appspot.com/6111059/diff/1/tests/preprocessor_tests/comment...
File tests/preprocessor_tests/comment_test.cpp (right):

http://codereview.appspot.com/6111059/diff/1/tests/preprocessor_tests/comment...
tests/preprocessor_tests/comment_test.cpp:61: }
On 2012/04/26 22:03:16, kbr1 wrote:
> Need tests ensuring that multi-line comments followed by errors report the
> correct line number for the error.

Will do.

[kbr1, 2012-04-26 23:06:48]

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp.l
File src/compiler/preprocessor/new/pp.l (right):

http://codereview.appspot.com/6111059/diff/1/src/compiler/preprocessor/new/pp...
src/compiler/preprocessor/new/pp.l:68: /* Line breaks are just counted - not
returned. */
On 2012/04/26 22:38:40, Alok Priyadarshi wrote:
> On 2012/04/26 22:03:16, kbr1 wrote:
> > On 2012/04/26 05:11:15, Alok Priyadarshi wrote:
> > > GLSL spec says "New-lines are not eliminated by comments". I am not sure
> > exactly
> > > what it means. If we keep the newlines inside comments for further
> processing
> > > then this is invalid:
> > > 
> > > # /* foo
> > > */ define bar 1
> > > 
> > > Here if we ignore the newline after foo then this is equivalent to "#
define
> > bar
> > > 1", otherwise it is invalid. gcc and VC++ consider it valid. For now I
have
> > > matched their behavior by just counting but removing them from further
> > > processing.
> > 
> > The reason for the wording in the GLSL spec is almost certainly to avoid
> having
> > multi-line comments mess up the line numbers reported in compilation errors.
> > 
> > If the preprocessor only counts line numbers but doesn't transmit them to
the
> > later parsing phase (for example, via #line directives) then any validation
> > errors will report incorrect line numbers.
> > 
> > I would not go to great lengths to make your use case of #defines split by
> > comments work correctly. It is absolutely essential, though, that the line
> > numbers match up all the way through the shader validation and translation.
> 
> What I have right now should not mess up the line numbers since we are
counting
> the line numbers at a common location. Even #line directive will update the
same
> yylineno variable in the lexer.

I understand, but the preprocessor is a completely different lexer/parser than
the shader validator or translator. If the preprocessor collapses lines then
later shader processing phases will report incorrect line numbers -- unless the
preprocessor does something like emit #line directives to fix them back up.

> So if I make sure that line number reporting is correct, what I have right now
> is correct?

Yes, if line number reporting will be correct in all kinds of errors (not just
those caught by the preprocessor's lexer) then LGTM.