Bug 78521 – The comment about terminating nulls at the top of CSSParser::lex() is incorrect

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Cris Neckar]

We should fix the comment 

// The input buffer is terminated by two \0, so
// it is safe to read two characters ahead anytime

at the top of CSSParser::lex(). It is wrong and suggests that there are 2 characters of nulls. This should probably say something like

// The input buffer is terminated by a \0 character, so
// it is safe to read one character ahead of a known non-null

[Cris Neckar]

Created attachment 127087 [details]
Patch

[Eric Seidel (no email)]

Difficult for me to review this, because I don't know enough about lex().   Who creates this buffer?  Bison?  Can we point to the bison docs where it describes the buffer it creates?

[Cris Neckar]

(In reply to comment #2)
> Difficult for me to review this, because I don't know enough about lex().   Who creates this buffer?  Bison?  Can we point to the bison docs where it describes the buffer it creates?

There is no real immediacy here as it is just a comment change. Zoltan can review this when he has time.

[David Barr]

I think the comment is right but the code is wrong.
CSSParser::lex depends on this invariant to detect the end of the buffer.

[Cris Neckar]

The one case where it depended on it was just changed. I'm not sure whether it was originally supposed to be one or 2 nulls but at this point only one is written and lex() only relies on there being one.

[David Barr]

Thanks for the clarification, I see that dependence on double-null was removed recently:
http://trac.webkit.org/changeset/107369

[Zoltan Herczeg]

Comment on attachment 127087 [details]
Patch

The double NULL originated from flex. The last comment here explains it:

http://stackoverflow.com/questions/1909166/how-to-parse-from-a-string-rather-than-a-file

When I started working on the lexer, I kept this behaviour first. Later I realized I didn't need double NULL terminator, but the comment was remained and the comment parsing was wrong.

[WebKit Review Bot]

Comment on attachment 127087 [details]
Patch

Clearing flags on attachment: 127087

Committed r108043: <http://trac.webkit.org/changeset/108043>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.