Sorry I didn't get to this yesterday.  This will now need to change after my
last change.

It also needs some ARM functions stubbed if you want to keep us building on ARM.


http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2116
src/hydrogen.cc:2116: void
HGraphBuilder::VisitForTypeofValue(Expression* expr) {
It seems to me that "parent is typeof" is just a flag on a value
context, so this function could be:

void HGraphBuilder::VisitForTypeofValue(Expression* expr) {
  ValueContext for_value(this);
  for_value.mark_as_typeof();
  Visit(expr);
}

That also dodges any problems with "HandleGlobalVariableLoad" (see
below).

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2118
src/hydrogen.cc:2118: if (proxy != NULL && !proxy->var()->is_this() &&
proxy->var()->is_global()) {
I think you can just ask the proxy is_this (and maybe is_global?).  If
you follow my suggestion above you don't need to do anything here,
though.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2957
src/hydrogen.cc:2957: void
HGraphBuilder::LookupGlobalPropertyCell(VariableProxy* expr,
It seems like it would be cleaner for this function to return a boolean
indicating whether it was found or not, rather than taking a pointer to
a flag.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2979
src/hydrogen.cc:2979: void
HGraphBuilder::HandleGlobalVariableLoad(VariableProxy* expr,
We need to restore some sanity to these helper functions.  There are
dozens of "Visit....", "Build....", and "Handle...." in here with no
consistent naming scheme.

I actually inlined this function and removed its definition in my last
change because I was about to put a comment to the effect of "This
function should only be called in tail position as part of visiting...."
and just decided to get rid of the (called once) helper.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2988
src/hydrogen.cc:2988: PushAndAdd(new
HLoadGlobalGeneric(expr->var()->name(), inside_typeof));
Here you could just ask the ast_context if it is a typeof context, which
is only true for value contexts with the typeof bit set.  There's no
need to even pass it to this helper if you set up the context of the
subexpression as suggested above.

http://codereview.chromium.org/5550003/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to