https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc#newcode1517
src/hydrogen.cc:1517: IfBuilder internalized(this);
On 2014/06/27 15:37:51, Jakob wrote:
nit: I think "not_internalized" would more accurately convey what it
checks
Done.
https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc#newcode1532
src/hydrogen.cc:1532: // Key guaranteed to be a unqiue string
On 2014/06/27 15:37:51, Jakob wrote:
nit: while you're here, "unique"
Done.
https://codereview.chromium.org/356213003/diff/40001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/356213003/diff/40001/src/runtime.cc#newcode1607
src/runtime.cc:1607: SealHandleScope shs(isolate);
On 2014/06/27 15:37:51, Jakob wrote:
no
Done.
https://codereview.chromium.org/356213003/diff/40001/src/runtime.h
File src/runtime.h (right):
https://codereview.chromium.org/356213003/diff/40001/src/runtime.h#newcode277
src/runtime.h:277: F(SetIteratorNext, 2, 1) \
On 2014/06/27 15:37:51, Jakob wrote:
no
Done.
https://codereview.chromium.org/356213003/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/356213003/diff/60001/src/hydrogen.cc#newcode1521
src/hydrogen.cc:1521: not_internalized.Then();
On 2014/06/27 17:37:37, Jakob wrote:
Now you've changed both the name and the meaning. (Or am I confused?)
Either the IfBuilder should be called "internalized", then the
condition should
be "not_internalized_bit == 0", and the Then() part should push the
key while
the Else() part calls Runtime_InternalizeString.
Or it should be called not_internalized, with the condition being
"not_internalized != 0", the Then() branch calling Internalize and the
Else()
branch pushing the original key (as the not_internalized bit is not
unequal to
0... quadruple negation ftw!).
It would probably make sense to add a
RUNTIME_ASSERT(!StringShape(*string)->IsInternalized()) to
Runtime_InternalizeString...
This is intentional. I also switched from Token::NE to Token::EQ. This
generates better code, since the "then" case which doesn't have to do
any work jumps over the "else" case that does. But your right, the name
is now flipped. I'll fix that
As far as the runtime assert, it's not needed, since Factory does the
right thing with already internalized strings.
https://codereview.chromium.org/356213003/
--
--
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.
