https://codereview.chromium.org/753553002/diff/40001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode516
include/v8.h:516: if (a == 0) return b == 0;
On 2014/11/28 12:15:11, picksi1 wrote:
Should the '0's be NULL's for consistency with the [Is]Empty()
functions above?
Done.
https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode517
include/v8.h:517: if (b == 0) return false;
On 2014/11/28 12:15:11, picksi1 wrote:
If you changed the two above if's with...
if (a==b) return true; // both are null, or point at the same thing
if ((a & b)==0) return false; // One is null
... you'd save having to deference the pointer if a & b both point at
the same
thing... Don't know if this is an impossible case though?
Not sure, but you definitely don't want bitwise & here :-)
https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode6144
include/v8.h:6144: static const int kNodeStateMask = 0x7;
On 2014/11/28 12:15:11, picksi1 wrote:
Can you generate this 0x7 mask? e.g.
kNodeStateMask = kNodeStateIsWeakValue | kNodeStateIsPendingValue |
kNodeStateIsNearDeathValue
... Or are those the wrong consts?
Could be, but I'm going to stay consistent with the style in the rest of
the file.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc
File src/global-handles.cc (right):
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode102
src/global-handles.cc:102: void Zap() {
On 2014/11/28 12:15:11, picksi1 wrote:
Is 'Zap' a known term in the world of GC? For the naive reader (i.e.
me) it
doesn't tell me what the function does! Should your comment inside the
function
be turned into the name? e.g. MarkForEagerTrapping()?
It's pretty well established, I think.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode166
src/global-handles.cc:166: flags_ = NodeWeaknessType::update(flags_, t);
On 2014/11/28 12:15:11, picksi1 wrote:
is t a good variable name here? should it be weakness_type?
Done.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode263
src/global-handles.cc:263: if (parameter != NULL) {
On 2014/11/28 12:15:11, picksi1 wrote:
Bikeshed: Might be clearer to swap the if/else bodies and turn the !=
into ==
Done.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode301
src/global-handles.cc:301: if (weakness_type() == Node::NORMAL_WEAK) {
On 2014/11/28 12:15:11, picksi1 wrote:
Could this code be refactored to switch on the weakness_type? It might
be [very
slightly!] quicker than the cascading ifs, and possible clearer?
Although you
may end up with duplicate phantom callback code in the PHANTOM_WEAK
and
INTERNAL_FIELDS_WEAK case.
I have to rewrite this anyway because I have worked out why it's
completely wrong.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode320
src/global-handles.cc:320: if (state() != FREE) {
On 2014/11/28 12:15:11, picksi1 wrote:
There are lots of 'state() != FREE's in the code, is it worth creating
a
function (state_not_free()?) that captures this concept?
Created IsInUse() for this.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode586
src/global-handles.cc:586: Node::FromLocation(location)
On 2014/11/28 12:15:11, picksi1 wrote:
Do internal_field_index1/2 have anything to do with the
internal_field1/2
elements in the union+struct on line 388 above? They are named almost
the same
but are different types, should they be "harmonised"?
Done.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode635
src/global-handles.cc:635: if (node->state() == Node::PENDING) {
On 2014/11/28 12:15:11, picksi1 wrote:
switch? Not sure if a 3 case switch beats 2 ifs on performance!
The compiler will certainly convert a 3 case switch into two 'if's. I
think the ifs are more readable.
https://codereview.chromium.org/753553002/
--
--
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.
