[webkit-reviews] review granted: [Bug 22385] Lazily reparse FunctionBodyNodes on first execution. : [Attachment 25320] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 20 11:50:42 PST 2008
Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 22385: Lazily reparse FunctionBodyNodes on first execution.
https://bugs.webkit.org/show_bug.cgi?id=22385
Attachment 25320: patch
https://bugs.webkit.org/attachment.cgi?id=25320&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> +//Program_NoNode:
> +// /* not in spec */
> +// | SourceElements_NoNode
> +//;
Lets not check that in.
> bool Lexer::isIdentStart(int c)
> {
> - return (category(c) & (Letter_Uppercase | Letter_Lowercase |
Letter_Titlecase | Letter_Modifier | Letter_Other))
> - || c == '$' || c == '_';
> + return isASCIIAlpha(c) || c == '$' || c == '_' || (category(c) &
(Letter_Uppercase | Letter_Lowercase | Letter_Titlecase | Letter_Modifier |
Letter_Other));
> }
A more efficient version of this would call "category" only if the character is
non-ASCII. A patch I wrote a while back added isASCII to <wtf/ASCIICType.h> for
just this sort of use.
I think you should land these smaller independent enhancements to Lexer
separately.
> static bool isDecimalDigit(int c)
> {
> - return (c >= '0' && c <= '9');
> + return isASCIIDigit(c);
> }
I don't see the point of making a function call for this every time. Could we
just eliminate this function instead and use isASCIIDigit directly? Or make it
inline?
> bool Lexer::isHexDigit(int c)
> {
> - return (c >= '0' && c <= '9'
> - || c >= 'a' && c <= 'f'
> - || c >= 'A' && c <= 'F');
> + return isASCIIHexDigit(c);
> }
>
> bool Lexer::isOctalDigit(int c)
> {
> - return (c >= '0' && c <= '7');
> + return isASCIIOctalDigit(c);
> }
Same comment.
> + for (size_t i = 0; i < size; ++i)
> + releaser.release(m_data->m_children[i]);
> +}
> +
> +
> // ------------------------------ ProgramNode -----------------------------
Extra blank line here.
> + // Eval code needs to hang on to its declaration stacks to keep
declaration info alive until Interpreter::execute time,
> + // so the entire ScopeNodeData cannot be destoyed.
> + children().shrinkCapacity(0);
Is this the right idiom? Didn't Maciej change clear() to do this?
> Index: parser/Nodes.h
> ===================================================================
> --- parser/Nodes.h (revision 38582)
> +++ parser/Nodes.h (working copy)
> @@ -170,6 +170,7 @@ namespace JSC {
> virtual bool isResolveNode() const JSC_FAST_CALL { return false; }
> virtual bool isBracketAccessorNode() const JSC_FAST_CALL { return
false; }
> virtual bool isDotAccessorNode() const JSC_FAST_CALL { return false;
}
> + virtual bool isFuncExprNode() const JSC_FAST_CALL { return false; }
>
> virtual ExpressionNode* stripUnaryPlus() { return this; }
>
> @@ -191,6 +192,7 @@ namespace JSC {
>
> virtual bool isEmptyStatement() const JSC_FAST_CALL { return false;
}
> virtual bool isReturnNode() const JSC_FAST_CALL { return false; }
> + virtual bool isExprStatementNode() const JSC_FAST_CALL { return
false; }
I don't think this needs "Node" in its name.
> + void setData(ScopeNodeData* data) { m_data.set(data); }
This should take an auto_ptr argument, in my opinion. We probably also want to
name it adoptData rather than setData.
> +static FunctionBodyNode* getFunctionBody(ProgramNode* program)
No need for "get" in the name of this function. You can just name the local
variable "body" to avoid the conflict.
r=me
More information about the webkit-reviews
mailing list