[webkit-reviews] review granted: [Bug 15884] JavaScriptCore should use simple type inferencing to speed-up AddNode : [Attachment 17181] Patch updated to TOT and addressing Darin's concerns

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Nov 11 06:52:43 PST 2007

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 15884: JavaScriptCore should use simple type inferencing to speed-up

Attachment 17181: Patch updated to TOT and addressing Darin's concerns

------- Additional Comments from Darin Adler <darin at apple.com>
 269 __ZN3KJS8JSObject18getPrimitiveNumberEPNS_9ExecStateERdRPNS_7JSValueE

Why does this need to be exported? Who calls it outside JavaScriptCore?

 925	 // Both are objects (or unknown) -- fixme: this is kinda an
inefficient way to get here.

I don't understand that comment, and the format seems strange.

 1928	  double n2 = term2->evaluateToNumber(exec);
 1931	  return n1 + n2;

I don't think you really need this exception check here since it's the last
call to evaluate in another evaluate. The inner evaluate will already have
attached the file and line number and there's no need to optimize out the

 1949	  JSValue* v2 = term2->evaluate(exec);

Same here.

 2135	  return jsBoolean(n1 < n2);

And here.

 2144	  return jsBoolean(static_cast<StringImp*>(v1)->value() <

and here.

 121	 JSType expectedReturnType() const KJS_FAST_CALL { return
m_expectedReturnType; }

Shouldn't this function move into ExpressionNode? I know that currently every
Node has m_expectedReturnType, but it would be nice not to make the promise for
the future.

 160	 JSType m_expectedReturnType : 3;

I think to make this work properly on Windows this needs to be unsigned, not

 253	   JSValue* m_value; // This is not a real JSValue, only ever a
JSImmediate, thus no ProtectedPtr

I would word it as "never a JSCell", rather than "not a real JSValue".

I don't understand the line of code in the LocalVarTypeOfNode constructor that
sets m_expectedReturnType to StringType. Isn't it guaranteed to already be

It seems AddNodeNumbers should be AddNumberNode, and AddNodeStrings should be
AddStringsNode. For AddNodeStringLeft and AddNodeStringRight, AddStringLeftNode
and AddStringRightNode are not great names, but probably OK unless we can think
of something better.

r=me as long as you fix the m_expectedReturnType issue.

More information about the webkit-reviews mailing list