[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