Closed Bug 656810 Opened 9 years ago Closed 8 years ago

Implement js::NumberObject

Categories

(Core :: JavaScript Engine, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
As object subclasses go it's on the low end of the importance scale, but we should have it to get away from the getPrimitiveThis/setPrimitiveThis punning.
Attachment #532079 - Flags: review?(nnethercote)
Comment on attachment 532079 [details] [diff] [review]
Patch

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

r=me

::: js/src/vm/NumberObject.h
@@ +69,5 @@
> +    }
> +
> +  private:
> +    inline void init(jsdouble d) {
> +        setSlot(PRIMITIVE_THIS_SLOT, NumberValue(d));

Is PRIMITIVE_THIS_SLOT the best name now?  Would something simpler like VALUE_SLOT be better?  And if you make that change, would setValue() be a better name than init()?

@@ +78,5 @@
> +    ::js_InitNumberClass(JSContext *cx, JSObject *global);
> +
> +  private:
> +    NumberObject();
> +    NumberObject &operator=(const NumberObject &so);

Add a comment here, please, explaining these are declared private so they can't be used.  (And maybe why you don't want them to be used.)
Attachment #532079 - Flags: review?(nnethercote) → review+
5 out 5 JS engineers just surveyed agree that private cctor/op= is a basic C++ idiom and not deserving of comment.
(In reply to comment #1)
> Is PRIMITIVE_THIS_SLOT the best name now?  Would something simpler
> like VALUE_SLOT be better?  And if you make that change, would
> setValue() be a better name than init()?

I had thought I was mimicking the name of the spec internal property, but that was actually [[PrimitiveValue]], so I renamed to PRIMITIVE_VALUE_SLOT and setPrimitiveValue.

> @@ +78,5 @@
> > +    ::js_InitNumberClass(JSContext *cx, JSObject *global);
> > +
> > +  private:
> > +    NumberObject();
> > +    NumberObject &operator=(const NumberObject &so);
> 
> Add a comment here, please, explaining these are declared private so
> they can't be used.  (And maybe why you don't want them to be used.)

A poll of the office says private constructor/operator= are idiomatic for "don't do that", so I'm leaving this as-is.  It doesn't seem important to clarify why, but anyone who's truly curious shouldn't have to think much past "this is GC-managed" to explain.

This is atop a bunch of other patches, so it'll be awhile before it lands, I think (but for 6, I believe).
...and four and a half months later, those other patches have made their way through reviews and such into the tree -- landed:

http://hg.mozilla.org/integration/mozilla-inbound/rev/66736ca9c8c3
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/66736ca9c8c3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.