890076 - Move isConstructing to CallArgs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Seems like a happier place for it to live.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Move isConstructing to CallArgs

You will, of course, object to the way I crammed in the asserts sideways. If only I understood the invariants it's asserting about.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 771051 [details] [diff] [review]
Move isConstructing to CallArgs

I guess it's ok with me if you hold off on your review until I make it stop crashing on ./js -e 1

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Move isConstructing to CallArgs

This is a little better.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 771063 [details] [diff] [review]
Move isConstructing to CallArgs

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

::: js/public/CallArgs.h
@@ +63,5 @@
>  
> +#ifdef DEBUG
> +extern JS_PUBLIC_API(void)
> +CheckIsValidConstructible(Value v);
> +#endif

In namespace JS::detail, please.

@@ +178,5 @@
>  
> +    bool isConstructing() const {
> +#ifdef DEBUG
> +        CheckIsValidConstructible(calleev());
> +#endif

Why don't we condition this test on |this->usedRval_|, so that every isConstructing() doesn't have to tiptoe around rval sets?

::: js/src/builtin/Intl.cpp
@@ +641,5 @@
>  static JSBool
>  Collator(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    return Collator(cx, args, args.isConstructing());

Erm, please go further and fix Collator and NumberFormat and DateTimeFormat to determine constructing-ness through args, not through a separately-passed argument.

::: js/src/jsfun.cpp
@@ +1155,5 @@
>  /* ES5 15.3.4.5.1 and 15.3.4.5.2. */
>  JSBool
>  js::CallOrConstructBoundFunction(JSContext *cx, unsigned argc, Value *vp)
>  {
> +    CallArgs callargs = CallArgsFromVp(argc, vp);

Use |args| here, and make the name of the other |args| be |invokeArgs| or something.

@@ +1763,5 @@
> +        JS_ASSERT(callee->as<JSFunction>().isNativeConstructor());
> +    else
> +        JS_ASSERT(callee->getClass()->construct != NULL);
> +}
> +}

Blank lines at the edges of namespace blocks, please.  And // namespace JS // namespace detail the namespace-closing braces, too.

::: js/src/jsfun.h
@@ +488,5 @@
>  
> +#ifdef DEBUG
> +namespace JS {
> +JS_PUBLIC_API(void)
> +CheckIsValidConstructible(Value calleev);

Put this in JS::detail, please, so it's more clearly internal.

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Comment on attachment 771063 [details] [diff] [review]
> Move isConstructing to CallArgs
> 
> Review of attachment 771063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/CallArgs.h
> @@ +63,5 @@
> >  
> > +#ifdef DEBUG
> > +extern JS_PUBLIC_API(void)
> > +CheckIsValidConstructible(Value v);
> > +#endif
> 
> In namespace JS::detail, please.

Ok

> @@ +178,5 @@
> >  
> > +    bool isConstructing() const {
> > +#ifdef DEBUG
> > +        CheckIsValidConstructible(calleev());
> > +#endif
> 
> Why don't we condition this test on |this->usedRval_|, so that every
> isConstructing() doesn't have to tiptoe around rval sets?

Sure, if you say so. You say gobbledygook, I write down gobbledygook. We both happy. Happy is good.

> ::: js/src/builtin/Intl.cpp
> @@ +641,5 @@
> >  static JSBool
> >  Collator(JSContext *cx, unsigned argc, Value *vp)
> >  {
> >      CallArgs args = CallArgsFromVp(argc, vp);
> > +    return Collator(cx, args, args.isConstructing());
> 
> Erm, please go further and fix Collator and NumberFormat and DateTimeFormat
> to determine constructing-ness through args, not through a separately-passed
> argument.

You didn't expect me to actually read the code I was patching, did you? Fixed.

> ::: js/src/jsfun.cpp
> @@ +1155,5 @@
> >  /* ES5 15.3.4.5.1 and 15.3.4.5.2. */
> >  JSBool
> >  js::CallOrConstructBoundFunction(JSContext *cx, unsigned argc, Value *vp)
> >  {
> > +    CallArgs callargs = CallArgsFromVp(argc, vp);
> 
> Use |args| here, and make the name of the other |args| be |invokeArgs| or
> something.

Ok

> @@ +1763,5 @@
> > +        JS_ASSERT(callee->as<JSFunction>().isNativeConstructor());
> > +    else
> > +        JS_ASSERT(callee->getClass()->construct != NULL);
> > +}
> > +}
> 
> Blank lines at the edges of namespace blocks, please.  And // namespace JS
> // namespace detail the namespace-closing braces, too.

Ok

Comment 6

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/39c322f034cc

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/39c322f034cc