877216 - Add CallArgs-like structs that we can use for specialized DOM methods/getters/setters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This would be much like CallArgs, but without extra baggage.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Add CallArgs-like structs for use in DOM specialized getters/setters/methods.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 755457 [details] [diff] [review]
Add CallArgs-like structs for use in DOM specialized getters/setters/methods.

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

Good modulo nits, and the static initializer question being answered in some satisfactory manner.

::: js/public/CallArgs.h
@@ +92,5 @@
>   * public interface are meant to be used by embedders!  See inline comments to
>   * for details.
>   */
> +
> +enum UsedRval { IncludeUsedRval, NoUsedRval };

Please put this enum, UsedRvalBase, CallReceiverBase, and CallArgsBase all inside namespace detail {} blocks, to clarify that embedders shouldn't use them directly.  Also add a comment noting that these classes are further used by jsfriendapi.h, so that people are aware of their being used outside CallArgs.h.

@@ +97,5 @@
> +
> +template<UsedRval WantUsedRval>
> +class MOZ_STACK_CLASS UsedRvalBase
> +{
> +};

I think you can do

template<UsedRval WantUsedRval>
class MOZ_STACK_CLASS UsedRvalBase;

and just declare the class, then let the subsequent specializations fill it out.  Doesn't much matter here (for enums or bools in general), but for classes in general it means a mis-parameterization will result in a not-defined error, versus using something unexpected.

@@ +216,5 @@
>  
> +
> +class MOZ_STACK_CLASS CallReceiver : public CallReceiverBase<IncludeUsedRval>
> +{
> + private:

One more space of indent here.  :-)

@@ +261,5 @@
>   * public interface are meant to be used by embedders!  See inline comments to
>   * for details.
>   */
> +template<UsedRval WantUsedRval>
> +class MOZ_STACK_CLASS CallArgsBase : public CallReceiverBase<WantUsedRval>

Instead of this, inherit from:

  public mozilla::Conditional<WantUsedRval == IncludeUsedRval,
                              CallReceiver,
                              typename CallReceiverBase<NoUsedRval> >::Type

...possibly with more or fewer typenames, and adding #include "mozilla/TypeTraits.h", would let you get rid of the conversion operator, which seems good to me.

@@ +310,5 @@
> +
> +class MOZ_STACK_CLASS CallArgs : public CallArgsBase<IncludeUsedRval>
> +{
> +  public:
> +    // We can look like a CallReceiver if we have to

http://www.youtube.com/watch?v=1I3pSZGCuHA

(also http://www.youtube.com/watch?v=vkk-sOo8Hqw if you require more quality education along those lines)

::: js/src/jsfriendapi.h
@@ +12,5 @@
>  #include "jsclass.h"
>  #include "jscpucfg.h"
>  #include "jspubtd.h"
>  #include "jsprvtd.h"
> +#include "js/CallArgs.h"

Blank line before this.  For reference, #include ordering in SpiderMonkey is

#include "ThisFile.h" // if you're in ThisFile.cpp, to know the header's self-contained

#include "mozilla/*"

#include "js*.h"

#include "*/*.h"

#include "js*inlines.h"

#include "*/*-inl.h"

@@ +1436,5 @@
>  /*
> + * A class, expected to be passed by value, which represents the CallArgs for a
> + * JSJitGetterOp.
> + */
> +class JSJitGetterCallArgs : protected JS::MutableHandleValue {

{ on its own line, also below

@@ +1439,5 @@
> + */
> +class JSJitGetterCallArgs : protected JS::MutableHandleValue {
> +  public:
> +    explicit JSJitGetterCallArgs(const JS::CallArgs& args) :
> +        JS::MutableHandleValue(args.rval())

We put : on the line after the constructor bit, indented two, so like so:

explicit JSBlah(...)
  : JS::MutableBlah(...)

@@ +1524,5 @@
> +    union {
> +        JSJitGetterOp getter;
> +        JSJitSetterOp setter;
> +        JSJitMethodOp method;
> +    };

Hmm.  Are you certain the casts that this requires will work without inducing static initializers?  I have this recollection that this will induce them.  I don't know how you can reasonably work around this, other than having three separate JSJitInfo structs and adding whatever overloads are needed to make all three work where only the current single type does now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Are you certain the casts that this requires will work without inducing static
> initializers?

No.  How do I tell?

> I don't know how you can reasonably work around this

I could leave the old setup, where we have to reinterpret_cast the "op" pointer when reading from the struct.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> No.  How do I tell?

Possibly glandium would know?

> I could leave the old setup, where we have to reinterpret_cast the "op"
> pointer when reading from the struct.

That seems safest and most expedient in the short term.

Comment 5

[Boris Zbarsky [:bzbarsky]]

A testcase .cpp file seems to indicate that initializing a union like this does not in fact need static initializers at least on Mac...

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> A testcase .cpp file seems to indicate that initializing a union like this
> does not in fact need static initializers at least on Mac...

clang is usually smart enough to forgo the static initializers; if you want to check, you should be checking on Linux.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing review comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f8d2127f2e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3f8d2127f2e