A first review.
https://codereview.chromium.org/883823002/diff/20001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/883823002/diff/20001/src/ast.h#newcode1633
src/ast.h:1633: return !is_this() && (!is_resolved() ||
var()->IsValidReference());
So it used to be that we would eagerly resolve "this" references, and we
would fall through to the second case where var()->IsValidReference()
returns false. But now that we create unresolved "this" references we
have to eagerly prevent unresolved "this" references from being valid
left-hand-sides.
Given that, it seems to me that we need to update Variable to not have
an is_valid_ref() field, as it's not needed any more -- the only cases I
could see where it mattered were for "this". (SuperExpression is not a
ThisExpression so the comment in variables.h is outdated.) Can you give
that a try? Then we remove the RHS of the && here.
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1178
src/scopes.cc:1178:
extra whitespace
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1206
src/scopes.cc:1206: // which is not preallocated.
Confusing comment. Also I think the conditional below should be
reassociated: if var->is_this(), then certainly var->is_used() (per the
if block above).
i.e. return var->is_this() || (var->is_used() &&
!var->IsGlobalObjectProperty());
https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1299
src/scopes.cc:1299: DCHECK(receiver()->scope() == this);
the second DCHECK is sufficient, I think, if we add the non-null DCHECK
to receiver() itself
https://codereview.chromium.org/883823002/diff/20001/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode149
src/scopes.h:149: bool is_this = false) {
Boolean parameters are an antipattern, especially optional boolean
parameters, because they read so poorly. Please create a small enum
somewhere. I know that is_this is used in other parts of the API but
it's bad there too.
https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode343
src/scopes.h:343: DCHECK(has_this_declaration());
DCHECK(receiver_) also perhaps?
https://codereview.chromium.org/883823002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
