[webkit-reviews] review requested: [Bug 14868] Import variable lookup optimizations from KJS : [Attachment 15958] Revised proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 14 04:30:22 PDT 2007

Cameron Zwarich (cpst) <cwzwarich at uwaterloo.ca> has asked  for review:
Bug 14868: Import variable lookup optimizations from KJS

Attachment 15958: Revised proposed patch

------- Additional Comments from Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>
(In reply to comment #2)
> (From update of attachment 15815 [edit])
> Need an extra space for indenting the comment at
> +  if (!body->builtSymbolList()) {
> +   // now handle other locals (functions, variables)
> +    body->processDecls(&newExec);


> I'm less keen on +  //### Document this, ugh.

I removed it. The same line it is commenting is without comment in the current
WebKit tree.

> function.h should also probably have you copyright info now


> ActivationImp
>   putLocal -> replace assert with ASSERT


>   putLocalChecked -> return on a new line


>   isLocalReadOnly -> i'm not sure but i believe VS will want
> !!(_locals[propertyID].attr & ReadOnly)


> VarAccessNode
>   optimizeLocalAccess -> no braces for "} else {" (and the closing '}')

Fixed. I also made it so there are no lone returns inside of else if and else

> NonLocalVarAccessNode
>   evaluateReference -> s/assert/ASSERT/


> nodes.h should possibly gain your copyright


> there are quite a few other places where assert is used instead of ASSERT

I changed them all.

> There are quite a few portions of this patch that look fairly trivial to make

> independent of this major patch -- for instance the [int] optimisation seems
> completely distinct from the improved scope lookup

Can you be more specific?

More information about the webkit-reviews mailing list