Add more FooObject classes

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(8 attachments, 3 obsolete attachments)

12.03 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
9.20 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
10.08 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
18.53 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
70.03 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
69.10 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.82 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
73.05 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Assignee

Description

6 years ago
Bug 880041 converted a lot of JSObject::{is,as}Foo() methods to JSObject::{is,as}<FooObject>().  But some remain because there isn't a FooObject class yet.  This bug is about introducing those FooObject classes.
Assignee

Comment 1

6 years ago
WeakMapObject is a pretty thin class, but it kills off isWeakMap() (in favour
of is<WeakMapObject>()) which is the goal.
Attachment #763907 - Flags: review?(jwalden+bmo)
Assignee

Comment 2

6 years ago
This makes WeakMapObject look complicated.

Getting IsStopIteration out of jsobjinlines.h is a win.
Attachment #763923 - Flags: review?(jwalden+bmo)
Assignee

Comment 3

6 years ago
Attachment #763928 - Flags: review?(jwalden+bmo)
Assignee

Comment 4

6 years ago
Whoops, forgot to |hg add| the new file.
Attachment #763932 - Flags: review?(jwalden+bmo)
Assignee

Updated

6 years ago
Attachment #763928 - Attachment is obsolete: true
Attachment #763928 - Flags: review?(jwalden+bmo)
Assignee

Comment 5

6 years ago
Attachment #763949 - Flags: review?(jwalden+bmo)
Assignee

Comment 6

6 years ago
The publicness of all the _SLOT constants is unfortunate, and could be fixed by 
making a lot of functions in jsdate.cpp methods in DateObject.cpp.  

But those slots were already public in JSObject and my goal here is to get rid
of isDate(), so I won't do that now.

ps: Bug 566789 sends its regards.
Attachment #763969 - Flags: review?(jwalden+bmo)
Comment on attachment 763907 [details] [diff] [review]
(part 1) - Add a WeakMapObject class.

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

::: js/src/jsweakmap.cpp
@@ +174,2 @@
>          map->clear();
>      }

Hmm, remove the braces while you're passing by.

::: js/src/vm/WeakMapObject.h
@@ +18,5 @@
> +{
> +  public:
> +    static Class class_;
> +
> +    ObjectValueMap *getMap() { return (ObjectValueMap *)getPrivate(); }

C++-style static_cast<>, please.

"getValueMap" might be slightly clearer, dunno.
Attachment #763907 - Flags: review?(jwalden+bmo) → review+
Attachment #763923 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 763932 [details] [diff] [review]
(part 3) - Add a GeneratorObject class.

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

::: js/src/vm/GeneratorObject.h
@@ +13,5 @@
> +{
> +  public:
> +    static Class class_;
> +
> +    JSGenerator *getGenerator() { return (JSGenerator *)getPrivate(); }

static_cast<>
Attachment #763932 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 9

6 years ago
This patch creates a minimal ArrayObject.  I'll move more stuff into it in the
next patch.
Attachment #764594 - Flags: review?(jwalden+bmo)
Assignee

Comment 10

6 years ago
This patch moves four methods from JSObject to ArrayObject, and also converts
various JSObject* args/retvals in surrounding functions to ArrayObject*.
Attachment #764626 - Flags: review?(jwalden+bmo)
Assignee

Comment 12

6 years ago
I don't understand how RegExpObject and RegExpStatics relate, but this patch
follows the pattern of previous patches and seems reasonable.
Attachment #765136 - Flags: review?(jwalden+bmo)
Comment on attachment 763949 [details] [diff] [review]
(part 4) - Add an ErrorObject class.

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

This is going to play havoc with the partial patchwork I have done for bug 724768.  I smell fun rebase times in my future...
Attachment #763949 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 763969 [details] [diff] [review]
(part 5) - Add a DateObject class.

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

Aughghgh this needs more accessors in another patch soon.
Attachment #763969 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 16

6 years ago
I made DateObject's slots private by moving more functions into DateObject.
This patch replaces the previous patch;  I was going to keep it separate but an
hg snafu meant I accidentally merged the patches.  It's actually not much
longer combined than the 2nd one would have been anyway.

To minimize churn, I kept all the new DateObject methods in jsdate.cpp rather
than moving them to DateObject.cpp.

I renamed various |date_getFoo_impl| functions as
|DateObject::date_getFoo_impl|.  Perhaps the |date_| prefix should be removed,
and maybe even the |_impl| suffix.  Not sure.

I used |date| as the name of DateObject objects, and thus had to rename some
existing |date| doubles as |ddate| (and I renamed some |dt| doubles as |ddate|
also, for consistency).
Attachment #766547 - Flags: review?(jwalden+bmo)
Assignee

Updated

6 years ago
Attachment #763969 - Attachment is obsolete: true
Comment on attachment 764594 [details] [diff] [review]
(part 6) - Add an ArrayObject class.

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

::: js/src/jsarray.cpp
@@ +2910,5 @@
>      if (!shape)
>          return NULL;
>  
> +    RootedObject obj(cx, JSObject::createArray(cx, allocKind, GetInitialHeap(newKind,
> +                                               &ArrayObject::class_), shape, type, length));

Splitting a method call, nested in function arguments, across two lines like you do for GetInitialHeap, is no good.  Put the whole GetInitialHeap call on a single line, please.
Attachment #764594 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 764626 [details] [diff] [review]
(part 7) - Move some methods from JSObject to ArrayObject.

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

::: js/src/jsarray.cpp
@@ +698,5 @@
>      uint32_t index, length;
>  
>      if (!js_IdIsIndex(id, &index))
>          return true;
> +    length = arr->length();

While you're here, it'd be nice to move the length declaration to here, and the index declaration to the line before the js_IdIsIndex call.

@@ +2155,4 @@
>       * as obj. This can only be performed if the original object is an array
>       * and has the same prototype.
>       */
> +    JS_ASSERT(narr->is<ArrayObject>());

I'd remove this assertion, its having been proven by induction, given how values of that type can be created.

::: js/src/jsobj.cpp
@@ +978,5 @@
>  bool
>  js::DefineProperty(JSContext *cx, HandleObject obj, HandleId id, const PropDesc &desc,
>                     bool throwError, bool *rval)
>  {
> +    if (obj->is<ArrayObject>()) {

Back last fall I wrote some patches to add methods to Handle<string type> to allow assertion-checked downcasts to Handle<more-derived string type>.  I never got around to landing them because I didn't have time to sniff out places to use them, but it was a perfectly sound idea.  Perhaps I should adapt those patches to work for Handle<object> and Handle<object subtype>, so we could use that instead of verbosely rerooting a bit in these sorts of cases.

::: js/src/jsobjinlines.h
@@ +557,5 @@
>  inline JSObject::EnsureDenseResult
>  JSObject::parExtendDenseElements(js::Allocator *alloc, js::Value *v, uint32_t extra)
>  {
>      JS_ASSERT(isNative());
> +    JS_ASSERT_IF(is<js::ArrayObject>(), as<js::ArrayObject>().lengthIsWritable());

If this file isn't including ArrayObject-inl.h, this lengthIsWritable() call will presumably trigger used-but-not-defined warnings anywhere that ArrayObject-inl.h isn't included first (which should be everywhere, given our include-order conventions).  So I think you need to include that file to avoid bootlegging anything.

::: js/src/vm/ArrayObject-inl.h
@@ +14,5 @@
> +
> +namespace js {
> +
> +/* static */ inline void
> +ArrayObject::setLength(JSContext *cx, js::Handle<ArrayObject*> arr, uint32_t length)

No need for js:: here, type lookup rules will mean Handle will be interpreted as js::Handle naturally.

::: js/src/vm/ArrayObject.h
@@ +15,5 @@
>  {
>    public:
>      static Class class_;
> +
> +    inline bool lengthIsWritable() const {

No need for inline on member functions that are actually defined (not just declared) inline -- it's implied by the syntactic form.

@@ +23,5 @@
> +    inline uint32_t length() const {
> +        return getElementsHeader()->length;
> +    }
> +
> +    static inline void setLength(JSContext *cx, js::Handle<ArrayObject*> arr, uint32_t length);

Same js:: thing here.
Attachment #764626 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 765136 [details] [diff] [review]
(part 8) - Add a RegExpStaticObject class.

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

I have a feeling the RegExp statics should really be in the compartment, or as reserved slots on the global, and there shouldn't be an object for this at all.  But I might be mistaken.  In any case this is certainly not the time to be making any changes along those lines.  :-)

::: js/src/vm/RegExpStatics.cpp
@@ +7,5 @@
>  #include "vm/RegExpStatics.h"
>  
>  #include "jsobjinlines.h"
>  
> +#include "vm/RegExpStaticsObject.h"

This should go above jsobjinlines.h.  (This probably would trigger that used-but-not-defined warning I mentioned in the last review.)
Attachment #765136 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 763949 [details] [diff] [review]
(part 4) - Add an ErrorObject class.

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

::: js/src/vm/ErrorObject.h
@@ +17,5 @@
> +{
> +  public:
> +    static Class class_;
> +
> +    JSExnPrivate *getExnPrivate() { return (JSExnPrivate *)getPrivate(); }

static_cast
Comment on attachment 766547 [details] [diff] [review]
(part 5) - Add a DateObject class.

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

There are few areas of SpiderMonkey I hate touching (or reviewing) more than jsdate.cpp, in terms of the volume of work involved in even the tiniest changes.  :-(

::: js/src/jsapi.cpp
@@ +64,5 @@
>  #include "ion/AsmJS.h"
>  #include "ion/PcScriptCache.h"
>  #include "js/CharacterEncoding.h"
>  #include "vm/Debugger.h"
> +#include "vm/DateObject.h"

Mis-alphabetized.

::: js/src/jsdate.cpp
@@ +1243,3 @@
>  {
> +    for (size_t ind = COMPONENTS_START_SLOT; ind < RESERVED_SLOTS; ind++)
> +        setSlot(ind, UndefinedValue());

You should use setReservedSlot here.  Really pretty much all the getSlot/setSlot with these should use the reserved variants.  So maybe/probably that should be a followup.  Up to you.

@@ +1874,5 @@
>      if (!GetMsecsOrDefault(cx, args, 1, t, &milli))
>          return false;
>  
>      /* Step 4. */
> +    double ddate = MakeDate(Day(t), MakeTime(HourFromTime(t), MinFromTime(t), s, milli));

Most of the |date| naming comes directly from the spec.  Instead of using |date| for the name of the object, could you instead use dateObj, so we don't have to deviate from the spec naming?

@@ +2117,4 @@
>          return false;
>  
>      /* Step 3. */
> +    double newDDate = MakeDate(MakeDay(YearFromTime(t), MonthFromTime(t), ddate), TimeWithinDay(t));

Same for |newDate| being a spec name.

::: js/src/vm/DateObject.h
@@ +40,5 @@
> +
> +  public:
> +    static Class class_;
> +
> +    inline const js::Value &getDateUTCTime() const {

I assume you're not getting rid of the "date" part of this to simplify your life for now?  Fine by me, but we should follow up and remove this stuff afterward.  (Also the "get" prefixes on these, when they're infallible.)

@@ +44,5 @@
> +    inline const js::Value &getDateUTCTime() const {
> +        return getFixedSlot(UTC_TIME_SLOT);
> +    }
> +
> +    inline void setDateUTCTime(const js::Value &time) {

Can this painlessly be Handle<Value>?

@@ +55,5 @@
> +    // If UTC time is not finite (e.g., NaN), the local time
> +    // slots will be set to the UTC time without conversion.
> +    void fillLocalTimeSlots(DateTimeInfo *dtInfo);
> +
> +    static JS_ALWAYS_INLINE bool date_getTime_impl(           JSContext *cx, CallArgs args);

Ugh that's fugly.  I'd really rather you didn't bother trying to vertically align any of this.  I might also get rid of the date_ bit of the names, too.

@@ +74,5 @@
> +    static JS_ALWAYS_INLINE bool date_getUTCMilliseconds_impl(JSContext *cx, CallArgs args);
> +    static JS_ALWAYS_INLINE bool date_getTimezoneOffset_impl( JSContext *cx, CallArgs args);
> +
> +    // Set UTC time to a given time and invalidate cached local time.
> +    void setUTCTime(double t, Value *vp = NULL);

Optionally-null arguments that are intended to be mutated are bad style, I think.  But this was pre-existing, so I guess we can keep running with it for now.

::: js/src/vm/SelfHosting.cpp
@@ +796,5 @@
>          RegExpObject &reobj = srcObj->as<RegExpObject>();
>          RootedAtom source(cx, reobj.getSource());
>          clone = RegExpObject::createNoStatics(cx, source, reobj.getFlags(), NULL);
> +    } else if (srcObj->is<DateObject>()) {
> +        clone = JS_NewDateObjectMsec(cx, srcObj->as<DateObject>().getDateUTCTime().toNumber());

I find myself wondering, in reading this, whether utcTime() shouldn't return a double.  No need to do anything here, and maybe it shouldn't (I haven't read any of the uses), just throwing it out there.
Attachment #766547 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 764594 [details] [diff] [review]
(part 6) - Add an ArrayObject class.

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

::: js/src/gc/Marking.cpp
@@ +1271,5 @@
>      uintptr_t start = stack.pop();
>      HeapSlot::Kind kind = (HeapSlot::Kind) stack.pop();
>  
>      if (kind == HeapSlot::Element) {
> +        if (obj->getClass() != &ArrayObject::class_)

Why not is<>() here?

::: js/src/jsobjinlines.h
@@ +322,1 @@
>      return getElementsHeader()->length;

So these move to ArrayObject later, right? :)
Assignee

Comment 24

6 years ago
> ::: js/src/jsobjinlines.h
> @@ +557,5 @@
> >  inline JSObject::EnsureDenseResult
> >  JSObject::parExtendDenseElements(js::Allocator *alloc, js::Value *v, uint32_t extra)
> >  {
> >      JS_ASSERT(isNative());
> > +    JS_ASSERT_IF(is<js::ArrayObject>(), as<js::ArrayObject>().lengthIsWritable());
> 
> If this file isn't including ArrayObject-inl.h, this lengthIsWritable() call
> will presumably trigger used-but-not-defined warnings anywhere that
> ArrayObject-inl.h isn't included first (which should be everywhere, given
> our include-order conventions).  So I think you need to include that file to
> avoid bootlegging anything.

lengthIsWritable() is defined in ArrayObject.h, not ArrayObject-inl.h.
Assignee

Comment 25

6 years ago
> There are few areas of SpiderMonkey I hate touching (or reviewing) more than
> jsdate.cpp, in terms of the volume of work involved in even the tiniest
> changes.  :-(

Then you'll like this:

 jsdate.cpp |  642 +++++++++++++++++++++++++------------------------------------
 1 file changed, 268 insertions(+), 374 deletions(-)

This is mostly due to less boilerplate code needed extracting the date info from |args|.


> ::: js/src/jsdate.cpp
> @@ +1243,3 @@
> >  {
> > +    for (size_t ind = COMPONENTS_START_SLOT; ind < RESERVED_SLOTS; ind++)
> > +        setSlot(ind, UndefinedValue());
> 
> You should use setReservedSlot here.  Really pretty much all the
> getSlot/setSlot with these should use the reserved variants.  So
> maybe/probably that should be a followup.  Up to you.

I'll do it in this patch -- all it's doing is adding assertions, so it's low risk.


> ::: js/src/vm/DateObject.h
> @@ +40,5 @@
> > +
> > +  public:
> > +    static Class class_;
> > +
> > +    inline const js::Value &getDateUTCTime() const {
> 
> I assume you're not getting rid of the "date" part of this to simplify your
> life for now?  Fine by me, but we should follow up and remove this stuff
> afterward.  (Also the "get" prefixes on these, when they're infallible.)

Oh, I just missed those.  I'll rename them.


> @@ +44,5 @@
> > +    inline const js::Value &getDateUTCTime() const {
> > +        return getFixedSlot(UTC_TIME_SLOT);
> > +    }
> > +
> > +    inline void setDateUTCTime(const js::Value &time) {
> 
> Can this painlessly be Handle<Value>?

Turns out setDateUTCTime() has just one caller, setUTCTime(), so I just inlined it.


> I find myself wondering, in reading this, whether utcTime() shouldn't return
> a double.  No need to do anything here, and maybe it shouldn't (I haven't
> read any of the uses), just throwing it out there.

In most cases we immediately call toNumber(), but there are three calls in jsdate.cpp where we don't do that.  /me shrugs.
Depends on: 887494
(In reply to Nicholas Nethercote [:njn] from comment #25)
> Then you'll like this:
> 
>  jsdate.cpp |  642
> +++++++++++++++++++++++++------------------------------------
>  1 file changed, 268 insertions(+), 374 deletions(-)
> 
> This is mostly due to less boilerplate code needed extracting the date info
> from |args|.

I'm not sure it's responsive to the fundamental issue.  The removals of the IsDate assertions are mostly meaningless, because they're clearly a second level of testing of the CallNonGenericMethod contract/promise, that the impl function will only be called with an acceptable |this|.  And if I remember right, at two lines a pop there, that's most of the reduction.  Not that it's *bad* to remove them, and implicitly do them in the as<> calls, but it does represent a deviation from how other impl functions have been written before.

The big problem with the date code is the underlying model is very finicky, and touching it at all usually requires touching every user.  It might be the nature of the beast; I don't have any good ideas.
Assignee

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee

Updated

6 years ago
Whiteboard: [js:t][leave open] → [js:t]
Assignee

Comment 29

6 years ago
Comment on attachment 766960 [details] [diff] [review]
(part 9) - Remove dead functions RenewProxyObject() and Wrapper::Renew().

I've moved this patch to bug 887558.
Attachment #766960 - Attachment is obsolete: true
Attachment #766960 - Flags: review?(ejpbruel)
You need to log in before you can comment on or make changes to this bug.