652215 - Implement js::StringObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

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.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 527858 [details] [diff] [review]
Patch

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?

Comment 2

[Brendan Eich [:brendan]]

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.

/be

Comment 3

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/24c1e19ddd50

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.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/24c1e19ddd50