Closed Bug 1147660 Opened 5 years ago Closed 5 years ago

Refactor NativeDefineProperty to separate ES6 invariant checks from implementation details

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(5 files)

No description provided.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Blocks: 1125624
Comment on attachment 8583447 [details] [diff] [review]
part 1 - Refactor NativeDefineProperty to put DefinePropertyOrElement's only call site right at the end. No change in behavior

Review of attachment 8583447 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. The fact that all this code used shape pointers as bools all over the place was deeply unsettling.
Attachment #8583447 - Flags: review?(efaustbmo) → review+
Comment on attachment 8583451 [details] [diff] [review]
part 2 - Merge DefinePropertyOrElement into NativeDefineProperty, making one long function we can refactor

Review of attachment 8583451 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED. rs=me

::: js/src/vm/NativeObject.cpp
@@ +1476,3 @@
>  }
>  
> +

nit: Is this added blank line intended?
Attachment #8583451 - Flags: review?(efaustbmo) → review+
Comment on attachment 8583453 [details] [diff] [review]
part 3 - Rearrange NativeDefineProperty so that special cases are all dispensed with, and ES6 checks done, by the time we start thinking about how to update the object

Review of attachment 8583453 [details] [diff] [review]:
-----------------------------------------------------------------

It's a little weird, from the outside, that a typed array is not is<ArrayObject>(), but it makes sense.
Attachment #8583453 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #7)
> nit: Is this added blank line intended?

Nope.
Comment on attachment 8583458 [details] [diff] [review]
part 4 - Change NativeDefineProperty to use a PropertyDescriptor internally instead of a bunch of variables. This is ugly at first but it'll get better

Review of attachment 8583458 [details] [diff] [review]:
-----------------------------------------------------------------

Not tht bad, after all.

::: js/src/vm/NativeObject.cpp
@@ +1248,2 @@
>  
> +    Rooted<PropertyDescriptor> desc(cx, desc_);

Hyper Nit: By the by, if we're going to use this all over, now. Should we consider adding typedefs for the rooted and handle types?

@@ +1280,5 @@
> +                desc.attributesRef() |= JSPROP_GETTER | JSPROP_SETTER;
> +                desc.assertComplete();
> +
> +                shape = NativeObject::changeProperty(cx, obj, shape, desc.attributes(),
> +                                                     desc.getter(), desc.setter());

This is already better.
Attachment #8583458 - Flags: review?(efaustbmo) → review+
Comment on attachment 8583461 [details] [diff] [review]
part 5 - Split the part of NativeDefineProperty that updates the object into a separate function again

Review of attachment 8583461 [details] [diff] [review]:
-----------------------------------------------------------------

I really like the merge-as-a-patch refactor-as-patches resplit-as-a-patch review flow in this patch. Thanks for making it easy on me.

::: js/src/vm/NativeObject.cpp
@@ -1407,5 @@
>      // have been dealt with.
> -
> -    desc.assertComplete();
> -    MOZ_ASSERT(desc.getter() != JS_PropertyStub);
> -    MOZ_ASSERT(desc.setter() != JS_StrictPropertyStub);

Where did the anti-stub asserts go? Is there a reason they were removed?
Attachment #8583461 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #11)
> Hyper Nit: By the by, if we're going to use this all over, now. Should we
> consider adding typedefs for the rooted and handle types?

Filed follow-up bug 1149534.
(In reply to Eric Faust [:efaust] from comment #12)
> > -    MOZ_ASSERT(desc.getter() != JS_PropertyStub);
> > -    MOZ_ASSERT(desc.setter() != JS_StrictPropertyStub);
> 
> Where did the anti-stub asserts go? Is there a reason they were removed?

Sorry, should have mentioned it. Those asserts are part of assertValid() which is part of assertComplete().
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.