Closed Bug 652215 Opened 11 years ago Closed 11 years ago

Implement js::StringObject


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)


(Whiteboard: fixed-in-tracemonkey)


(1 file)

Attached patch PatchSplinter Review
Note that this patch moves away from using the getPrimitiveThis/setPrimitiveThis stuff.  They're a bit type-punny, for starters, and they're actually usually the wrong answer -- but not always.  It turns out most places that get a Number do ToNumber on it to convert to number, and mutatis mutandis for String, yet getPrimitiveThis doesn't implement those semantics.  Yet for Boolean, when a boolean value is needed, getPrimitiveThis is right!  I'd like to move us toward having the getPrimitiveThis stuff be only on derived classes of objects, to help partly combat this problem.  This doesn't take us all the way, as we still do the getPrimitiveThis thing in a couple places, but it's a big enough step to provide the abstractions without being so big as to start making the patch unwieldy.

I'll likely continue on to Number and Boolean next, but because this work requires the sort of init-refactoring that I've only done to RegExp and String so far, I have other yaks to shave before doing so.
Attachment #527858 - Flags: review?(nnethercote)
Comment on attachment 527858 [details] [diff] [review]

Review of attachment 527858 [details] [diff] [review]:

I love this new Splinter review system!

r=me with nits addressed.

::: js/src/jsscopeinlines.h
@@ +159,1 @@
+    setSlot(PRIMITIVE_THIS_SLOT, StringValue(str));

Same complaint I made w.r.t. GlobalObject:  I'd like a slot-specific setter here please.  The obvious name is setPrimitiveThis which overlaps with the other one which isn't great... I'm sure you can come up with something :)

@@ +160,2 @@
     JS_ASSERT(str->length() <= JSString::MAX_LENGTH);
+    setSlot(LENGTH_SLOT, Int32Value(int32(str->length())));

Same here;  'setLength' is the obvious name.

::: js/src/jsstr.h
@@ +1269,5 @@
+    JSString *unbox() const {
+        return getSlot(PRIMITIVE_THIS_SLOT).toString();
+    }
+    inline size_t length() const {

I have a preference for 'get' prefixes on getters, ie. 'getLength'.  This is what I did in bug 566789 and we have obj->getArrayLength() and lots of others like it (obviously that would be aobj->getLength() when Array is sub-classed).

I realize both styles are in use in SM, so I'll leave the final naming up to you, but explicit 'get' prefixes are being used for slot-related getters already so there's a precedent there.

@@ +1289,5 @@
+    const js::Shape *assignInitialShape(JSContext *cx);
+  private:
+    StringObject();
+    StringObject &operator=(const StringObject &so);

This prevents you from assigning a StringObject, right?  Is it worth commenting on, eg. why we forbid it?
FYI, length() is an infallible getter whose C++ return value is it's result.

getLength() in Mozilla house style would be a fallible getter. That might mean an out param with a Boolean return value, worst case.


I moved the new files into vm/ before landing, per the recent js-internals thread.

I tend to think a slot-specific setter, if it would be called from only one location (as here), is just abstractionitis.  But I also don't really care, so I added in a setter that takes a JSString* and sets the primitive-slot and the length-slot based on it.

Mozilla's style is more in the line of not prefixing with get when it's infallible, as Brendan says.  I left this as it was.

Commenting the private operator= seems unnecessary, it's just usual belt-and-suspenders thinking for C++ classes that aren't assignable.  A part of me doesn't even think it's necessary, but it also doesn't hurt, so meh.
Whiteboard: fixed-in-tracemonkey
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #527858 - Flags: review?(nnethercote) → review+
You need to log in before you can comment on or make changes to this bug.