Last Comment Bug 652215 - Implement js::StringObject
: Implement js::StringObject
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-22 14:11 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-05-04 18:31 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.99 KB, patch)
2011-04-22 14:11 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
n.nethercote: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-22 14:11:15 PDT
Created attachment 527858 [details] [diff] [review]
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 [:njn] 2011-04-27 18:27:09 PDT
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] 2011-04-27 19:38:45 PDT
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] (remove +bmo to email) 2011-04-28 17:13:02 PDT
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) 2011-05-02 16:01:40 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/24c1e19ddd50

Note You need to log in before you can comment on or make changes to this bug.