Bug 71138 – Towards 8-bit Strings: Move Lexer and Parser Objects out of JSGlobalData

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

Currently JSGlobalData has Lexer and Parser objects that are used for JavaScript parsing.  These "singletons" are reset and reused for each parse task. As part of the overall 8-bit string changes, the Lexer (and therefore Parser) will be templatized on the character storage size and therefore need to be restructured.  Since parsing if a localized task (primarily in JavaScriptCore/runtime/Executable.cpp) it makes sense to create, use and destroy parser objects as needed.

<rdar://problem/10225268>

[Michael Saboff]

Created attachment 112908 [details]
Proposed patch

[Early Warning System Bot]

Comment on attachment 112908 [details]
Proposed patch

Attachment 112908 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10237347

[Gustavo Noronha (kov)]

Comment on attachment 112908 [details]
Proposed patch

Attachment 112908 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10236399

[Michael Saboff]

(In reply to comment #3)
> (From update of attachment 112908 [details])
> Attachment 112908 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/10236399

Yeah, I forgot to remove JSParser.{cpp,h} from all build files.  This looks like the source of the qt, gtk, win and likely efl build issues.

I'll provide an updated patch with speculative changes for these platforms.

[Gyuyoung Kim]

Comment on attachment 112908 [details]
Proposed patch

Attachment 112908 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10236406

[Michael Saboff]

Created attachment 112935 [details]
Updated patch with speculative build fixes

[Darin Adler]

Comment on attachment 112935 [details]
Updated patch with speculative build fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=112935&action=review

The thing I don’t like here is that some day we might want the source code location information to be more precise than just a line number. If we do, the we have to go back again and change all these call sites to pass something other than a line number. Is there some way we can avoid being so explicit about the line number? Any reason not to use something in global data instead of passing the arguments? Or could we at least pass a struct that has a line number in it instead of just passing the int?

> Source/JavaScriptCore/parser/ASTBuilder.h:146
> +    ExpressionNode* makeAssignNode(int, ExpressionNode* left, Operator, ExpressionNode* right, bool leftHasAssignments, bool rightHasAssignments, int start, int divot, int end);
> +    ExpressionNode* makePrefixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> +    ExpressionNode* makePostfixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> +    ExpressionNode* makeTypeOfNode(int, ExpressionNode*);
> +    ExpressionNode* makeDeleteNode(int , ExpressionNode*, int start, int divot, int end);
> +    ExpressionNode* makeNegateNode(int, ExpressionNode*);
> +    ExpressionNode* makeBitwiseNotNode(int, ExpressionNode*);
> +    ExpressionNode* makeMultNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeDivNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeModNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeAddNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeSubNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeBitXOrNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeBitAndNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeBitOrNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeLeftShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeRightShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeURightShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);

These should all say lineNumber. It’s not at all obvious that the integer is a line number. Also, there is an extra space on the makeDeleteNode line.

[Michael Saboff]

(In reply to comment #7)
> (From update of attachment 112935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112935&action=review
> 
> The thing I don’t like here is that some day we might want the source code location information to be more precise than just a line number. If we do, the we have to go back again and change all these call sites to pass something other than a line number. Is there some way we can avoid being so explicit about the line number? Any reason not to use something in global data instead of passing the arguments? Or could we at least pass a struct that has a line number in it instead of just passing the int?

I agree that it may make sense to have greater context, e.g. line number and position or column number, but I don't want to complicate the code for a possible future enhancement. I also think it would be safer not to use JSGlobalData as the source of that context.  This patch moves the source of the line number, the Lexer object, out of the JSGlobalData. Those two reasons suggest to me that we replace the lineNumber parameter with a context object at some future date.

> > Source/JavaScriptCore/parser/ASTBuilder.h:146
> > +    ExpressionNode* makeAssignNode(int, ExpressionNode* left, Operator, ExpressionNode* right, bool leftHasAssignments, bool rightHasAssignments, int start, int divot, int end);
> > +    ExpressionNode* makePrefixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> > +    ExpressionNode* makePostfixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> > +    ExpressionNode* makeTypeOfNode(int, ExpressionNode*);
> > +    ExpressionNode* makeDeleteNode(int , ExpressionNode*, int start, int divot, int end);
...
> > +    ExpressionNode* makeURightShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> 
> These should all say lineNumber. It’s not at all obvious that the integer is a line number. Also, there is an extra space on the makeDeleteNode line.

I added "lineNumber" to those methods as well as eliminated the extraneous space.

[Michael Saboff]

Committed r98887: <http://trac.webkit.org/changeset/98887>

[Yong Li]

The patch missed the definition of VectorTraits<JSParser::Scope>, which caused problem. See Bug 78932.