Closed Bug 773049 Opened 11 years ago Closed 11 years ago

Abstract away value forwarding in value wrappers


(Core :: JavaScript Engine, defect)

Other Branch
Not set





(Reporter: bhackett1024, Assigned: Waldo)


(Whiteboard: [js:t])


(2 files)

We have two classes right now --- Handle<Value> and EncapsulatedValue --- that basically copy-paste the innards of Value in order to forward .whatever() methods to a Value which they wrap.  We also need the same thing for Rooted<Value> and MutableHandle<Value>.  The duplicated code is nasty and makes changes to Value and the other classes error prone, infastructurey and not-likely-to-change-much as they are.  It would be nice if there was a common mechanism that could be shared by all these instances, whether through templates, subclassing or (groan) macros.
I need MutableHandle<Value> for other reasons and was already working on this sort of thing, so here's a patch.  The reason Handle<Value> is moved is because I want MutableHandle<Value> eventually for CallArgs, so the code needs to move around to not provoke any used-but-not-defined warnings.

I tested the effect of this on compilation time.  Without the patch, a -j1 build takes 7:35 on my system; with the patch it takes 7:51.  Without the patch, a -j8 build takes 2:32, and with it takes 2:31.  So it seems the effect on compilation time of this is happily in the noise.
Assignee: general → jwalden+bmo
Attachment #641254 - Flags: review?(bhackett1024)
Comment on attachment 641254 [details] [diff] [review]
Patch (also introduces a MutableHandle<Value> specialization)

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

::: js/src/gc/Barrier.h
@@ +303,4 @@
>  template <class T>
>  struct DefaultHasher< HeapPtr<T> > : HeapPtrHasher<T> { };
> +class EncapsulatedValue : public ValueOperations<EncapsulatedValue>

Aren't there still a bunch of Value methods in this class that haven't been removed?

::: js/src/jsapi.h
@@ +876,5 @@
> +};
> +
> +/* This is a specialization of the general Handle template in gc/Root.h. */
> +template <>
> +class Handle<Value> : public ValueOperations<Handle<Value> >

I was hoping to avoid duplication of the guts of Handle and MutableHandle as well.  I think you could fix this here by avoiding template specialization and just having a class HandleValue which inherits from Handle<Value> and ValueOperations<...>.  That means that explicit uses of Handle<Value> won't get the .whatever() methods and HandleValue needs to be used, but oh well.

@@ +958,5 @@
> +    void setBoolean(bool b) { ptr->setBoolean(b); }
> +    void setMagic(JSWhyMagic why) { ptr->setMagic(why); }
> +    bool setNumber(uint32_t ui) { return ptr->setNumber(ui); }
> +    bool setNumber(double d) { return ptr->setNumber(d); }
> +    void setObjectOrNull(JSObject *arg) { ptr->setObjectOrNull(arg); }

Can you factor these into a MutableValueOperations?  These will also be needed for RootedValue.  (Can you add that class too?)
Attachment #641254 - Flags: review?(bhackett1024)
Attached patch Patch, v2Splinter Review
Hmm, yes, MutableValueOperations is a good idea.

Thinking about specializing all this stuff, it occurs to me the previous patch was perhaps misled by how Handle<Value> had previously been specialized.  Rather than specializing Handle, and having to match up the signature of the default Handle template (and its constructors, and copying its internals), we should put the specialization into a *base class* of Handle.  Then the signature and implementation stay the same with no effort at all, and by construction the changes can only be additions.
Attachment #641509 - Flags: review?(bhackett1024)
Comment on attachment 641509 [details] [diff] [review]
Patch, v2

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

Attachment #641509 - Flags: review?(bhackett1024) → review+
Whiteboard: [js:t]
After a try-push to verify it compiled everywhere, I landed this in m-i:
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla16
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.