Move isConstructing to CallArgs

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Seems like a happier place for it to live.
You will, of course, object to the way I crammed in the asserts sideways. If only I understood the invariants it's asserting about.
Attachment #771051 - Flags: review?(jwalden+bmo)
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
Attachment #771051 - Flags: review?(jwalden+bmo)
This is a little better.
Attachment #771063 - Flags: review?(jwalden+bmo)
Attachment #771051 - Attachment is obsolete: true
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.
Attachment #771063 - Flags: review?(jwalden+bmo) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/39c322f034cc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 918462
You need to log in before you can comment on or make changes to this bug.