Bug 78521

Summary: The comment about terminating nulls at the top of CSSParser::lex() is incorrect
Product: WebKit Reporter: Cris Neckar <cdn>
Component: CSSAssignee: Cris Neckar <cdn>
Status: RESOLVED FIXED    
Severity: Trivial CC: davidbarr, eric, macpherson, menard, webkit.review.bot, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Description Cris Neckar 2012-02-13 13:21:53 PST
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
Comment 1 Cris Neckar 2012-02-14 17:19:34 PST
Created attachment 127087 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-16 13:36:24 PST
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?
Comment 3 Cris Neckar 2012-02-16 14:23:46 PST
(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.
Comment 4 David Barr 2012-02-16 16:31:08 PST
I think the comment is right but the code is wrong.
CSSParser::lex depends on this invariant to detect the end of the buffer.
Comment 5 Cris Neckar 2012-02-16 16:40:19 PST
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.
Comment 6 David Barr 2012-02-16 16:56:47 PST
Thanks for the clarification, I see that dependence on double-null was removed recently:
http://trac.webkit.org/changeset/107369
Comment 7 Zoltan Herczeg 2012-02-16 22:55:32 PST
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.
Comment 8 WebKit Review Bot 2012-02-16 23:48:14 PST
Comment on attachment 127087 [details]
Patch

Clearing flags on attachment: 127087

Committed r108043: <http://trac.webkit.org/changeset/108043>
Comment 9 WebKit Review Bot 2012-02-16 23:48:20 PST
All reviewed patches have been landed.  Closing bug.