Closed Bug 845478 Opened 7 years ago Closed 6 years ago

JS shell should use JS::CallArgs instead of manual argc/vp+JS_SET_RVAL/JS_ARGV/etc.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Waldo, Assigned: poiru)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [lang=c++][mentor=jwalden])

Attachments

(13 files, 44 obsolete files)

6.40 KB, patch
Ms2ger
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
26.52 KB, patch
Waldo
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
28.25 KB, patch
Waldo
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
26.31 KB, patch
Waldo
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
14.60 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
12.70 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
22.32 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
19.44 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
20.36 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
11.51 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.06 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
9.12 KB, patch
Waldo
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.05 KB, patch
poiru
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
We have to change the entire tree to CallArgs, and away from argc/vp and the JS_SET_RVAL/JS_CALLEE/JS_THIS/JS_ARGV macros, so we can change JSNative to take CallArgs& rather than argc and vp both.  Much of the JS engine's switched now in incremental updates, but the shell lags considerably.  So the shell needs concentrated fixing.

This is a pretty good first bug for someone to get interested in SpiderMonkey bugfixing, I think.  Happy to review patches here -- probably start with a small patch (or with questions, if you have them), get an okay on that, then go crazy.  :-)
I would love to do this and get started hacking spider monkey but I have no idea what needs to be done here. can you point me too some docs (I know about gecko c++ hacking).
Status: NEW → ASSIGNED
QA Contact: jviereck.dev
Great!  Unfortunately we don't really have docs for JS::CallArgs yet.  But the implementation of the class is super-simple -- see js/src/jsapi.h -- so it shouldn't be too bad.

The gist is that every function that's a JSNative -- that is, like so:

JSBool
FunctionName(JSContext *cx, unsigned argc, Value *vp)
{
    ...
}

(or jsval *vp for the last argument, jsval is a typedef to JS::Value) should change so that internally, it doesn't use argc/vp at all except to create a CallArgs, as the very first statement of the function:

    CallArgs argc = CallArgsFromVp(argc, vp);

Then, every place that currently uses |argc| should instead use |args.length()|.  And every place that uses vp should use one of a couple things.

JS_ARGV(cx, vp)[i] should change to args[i]
JS_SET_RVAL(cx, vp, value) should change to args.rval().set(value)
JS_CALLEE(cx, vp) should change to args.callee() (maybe a toObject() call is needed in there, I can't remember)
JS_THIS(cx, vp) should change to args.thisv()
JS_THIS_OBJECT(cx, vp)...hmm.  Leave this one alone for now, I'm not sure it's quite ready for conversion.
JS_RVAL(cx, vp) should ideally go away.  If it's ever used, args.rval() should be used to copy into a local variable, then that variable can be used as desired.  Use js::RootedValue v(cx, args.rval()) to store it, with whatever variable name makes sense.

Note that whenever JS_CALLEE is used, it has to be used before args.rval().  That *should* be the case already, but if it's not, a totally mechanical change would cause assertion failures.  Be careful, and watch out just in case.

As far as building SpiderMonkey goes, it's pretty simple:

  cd js/src
  autoconf-2.13 # or however it's named on your platform
  mkdir objdir
  cd objdir
  ../configure --enable-debug --disable-optimize
  make -s -j4

That'll give you a shell in objdir/js.  From js/src, you can run tests with:

  python tests/jstests.py objdir/js
  python jit-test/jit-test.py objdir/js

You can run subsets of tests as well by putting a substring of the test file name at the end of the command line.

If you need to know anything else, feel free to ask here or on #jsapi on IRC.

Everything cleared up?
Jeff, thanks a lot for your detailed comment! Looks good enough for me to get started on this.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> 
>     CallArgs argc = CallArgsFromVp(argc, vp);
> 

I think you mean:
     CallArgs args = CallArgsFromVp(argc, vp);
Er, yes.  :-)
Bug 846976 has a patch documenting CallArgs and how to use it which may be helpful in understanding what needs doing here.  I don't anticipate it changing much, if at all, between now and the time it lands.  I suspect it mostly restates what was said in comment 2, but there might be some new stuff in it.
I plan to work on this tomorrow/Sunday evening. Thanks a lot for providing such a good outline of what needs to be done here - this really makes we want to do this!
I cannot compile from the objdir using the snippets from Comment #2. See error output below. Any ideas?

  File "/Volumes/develop/moz/js/callArgs-845478/js/src/config/expandlibs.py", line 47, in splitpath
    dir, file = os.path.split(path)
  File "/Volumes/develop/moz/js/callArgs-845478/js/src/objdir/_virtualenv/bin/../lib/python2.7/posixpath.py", line 85, in split
    if head and head != '/'*len(head):
RuntimeError: maximum recursion depth exceeded in cmp
make[1]: *** [libmozjs.dylib] Error 1
make: *** [default] Error 2
Try running this in js/src, as my best guess at a fix, then retry the other build steps:

find . -name *.pyc | xargs rm
I got things compiling by avoiding '//' at the beginning of the path variable in `expandlibs.py` using the following code: 

    def splitpath(path):
        if (path.startswith('//')):        << NEW CODE
          path = path[1:]                  << NEW CODE
        dir, file = os.path.split(path)
        if os.path.splitdrive(dir)[1] == os.sep:
            return [file]
        print 'relativize-splitpath: %s, %s' % (dir, file)
        return splitpath(dir) + [file]


Does this makes any sense? The failing string was of the form '//usr/bin/clang'.

With these changes I the following errors running the tests:

$ python tests/jstests.py objdir/js

REGRESSIONS
    ecma/Date/15.9.5.35-1.js
    js1_5/extensions/toLocaleFormat-01.js
    js1_5/Regress/regress-58116.js
    js1_8/genexps/regress-380237-01.js

Is this something to worry about?

> python jit-test/jit-test.py objdir/js

I guess the file "jit-test.py" should be "jit_test.py"?

  python jit-test/jit-test.py objdir/js

All tests pass here :)
> REGRESSIONS
>     ecma/Date/15.9.5.35-1.js
>     js1_5/extensions/toLocaleFormat-01.js
>     js1_5/Regress/regress-58116.js
>     js1_8/genexps/regress-380237-01.js
> 
> Is this something to worry about?

No, those tests will keep failing for you. :( Partly, they rely on a specific locale, partly they only work in PST (I think). Would certainly be nice to fix them for us non-Californians (and -Oregonians).
Unassign myself - sigh, but it doesn't look promising I can get to this very soon and I don't want to keep this mentor bug reserved.
QA Contact: jviereck.dev
Status: ASSIGNED → NEW
hi, my name is Do Nhat Minh. I'm interested in fixing this bug. Can I get assigned to this bug?
Assignee: general → mrordinaire
Sure, go for it.  If you need anything beyond comment 2 and js/public/CallArgs.h, please comment here.  I'm a bit loaded these days, but if you ask here -- or on IRC in #jsapi, plenty of friendly people there to answer questions -- *someone* should answer relatively quickly.
I will. Thanks.
Hi, I have submitted a fix. Please checkout my patch. Any suggestions are welcomed.

While testing, these regressions are reported. I trust that they are not related?

> REGRESSIONS
>     js1_5/extensions/toLocaleFormat-01.js
>     js1_5/Regress/regress-58116.js
>     ecma/Date/15.9.5.24-2.js
>     ecma/Date/15.9.5.5.js
>     ecma/Date/15.9.3.2-1.js
>     ecma/Date/15.9.5.32-1.js
>     ecma/Date/15.9.5.36-3.js
>     ecma/Date/15.9.5.25-1.js
>     ecma/Date/15.9.5.24-3.js
>     ecma/Date/15.9.5.36-7.js
>     ecma/Date/15.9.5.36-5.js
>     ecma/Date/15.9.5.22-2.js
>     ecma/Date/15.9.5.33-1.js
>     ecma/Date/15.9.5.23-1.js
>     ecma/Date/15.9.5.6.js
>     ecma/Date/15.9.5.36-6.js
>     ecma/Date/15.9.5.23-17.js
>     ecma/Date/15.9.5.24-4.js
>     ecma/Date/15.9.5.24-8.js
>     ecma/Date/15.9.5.23-11.js
>     ecma/Date/15.9.5.8.js
>     ecma/Date/15.9.3.2-3.js
>     ecma/Date/15.9.5.27-1.js
>     ecma/Date/15.9.3.8-5.js
>     ecma/Date/15.9.5.35-1.js
>     ecma/Date/15.9.5.23-10.js
>     ecma/Date/15.9.5.16.js
>     ecma/Date/15.9.5.28-1.js
>     ecma/Date/15.9.5.22-4.js
>     ecma/Date/15.9.5.23-16.js
>     ecma/Date/15.9.5.22-1.js
>     ecma/Date/15.9.5.24-7.js
>     ecma/Date/15.9.5.24-5.js
>     ecma/Date/15.9.5.36-1.js
>     ecma/Date/15.9.5.37-1.js
>     ecma/Date/15.9.5.22-3.js
>     ecma/Date/15.9.5.29-1.js
>     ecma/Date/15.9.5.23-13.js
>     ecma/Date/15.9.3.8-1.js
>     ecma/Date/15.9.3.1-1.js
>     ecma/Date/15.9.5.30-1.js
>     ecma/Date/15.9.5.37-5.js
>     ecma/Date/15.9.3.8-2.js
>     ecma/Date/15.9.5.23-15.js
>     ecma/Date/15.9.5.26-1.js
>     ecma/Date/15.9.5.24-1.js
>     ecma/Date/15.9.3.1-4.js
>     ecma/Date/15.9.5.36-4.js
>     ecma/Date/15.9.5.24-6.js
>     ecma/Date/15.9.5.36-2.js
>     ecma/Date/15.9.5.31-1.js
>     ecma_5/Date/15.9.4.2.js
Attached patch Patch V2 (obsolete) — Splinter Review
Oops, I forgot about argc.

Attached is the new patch that also replaces `argc` in the code. The remaining instances of `argc` are more subtle, so I didn't touch them.

This new patch also has the above mentioned regressions.
Attachment #727273 - Attachment is obsolete: true
Attached patch Patch V3 (obsolete) — Splinter Review
This patch revert back to using JS_THIS_OBJECT, which is in the requirement.
Attachment #727526 - Attachment is obsolete: true
Attachment #727539 - Flags: review?(jwalden+bmo)
Wow, you went a lot further than I'd meant this bug to go!  I'd only meant to change js/src/shell/js.cpp, and leave the rest to other bugs.  It's great to see you do this to (apparently) all of SpiderMonkey!

I'll leave slightly more substantive responses to subsequent comments.  I just wanted to get out there that it's great to see you doing so much more than I'd expected, before I dive into preliminary thoughts on the patch.
This is a biiiiiiig patch.  :-)  Might I be able to convince you to split it up into several patches?  It's much easier to get reviews on ten 20K patches, even all from the same person (although I might try to enlist a couple other people to review some of the patches here, to keep things moving), than it is to get one review on a 200K patch.  It's easier to finish and submit a small review, than it is to keep chewing off bits and pieces of a really large patch until it's all complete.  This works both ways -- if I have comments, I can give you a small set of changes to make to a small patch, versus a possibly-huge list for a huge patch.  Plus, we can land early parts even if later parts haven't been reviewed yet.

Now, assuming you're willing to do that, how to split it a little.

I happened to notice that IsConstructing is a standalone method right now.  This wasn't part of the original bug, but could I maybe convince you to move IsConstructing() to CallReceiver::isConstructing(), and to update the documentation in CallArgs.h for the change?  It seems clear on a closer look that these methods should live in CallReceiver, not on their own.

Once that's done, I think the rest of this can be split up pretty much any way you want.  If it were me, I'd probably try to do half of js/src/shell/js.cpp in one patch, the other half in another, half of js/src/ctypes/CTypes.cpp in one patch, the other half in another, and all the remaining changes in one last patch.

All these changes are relatively simple, so *in theory* they're easy reviews.  But it's hard to maintain full attention through a too-long patch.  So splitting up further is better than not splitting up.
Thanks for the kind comments. I will get to it when I get back to my place.
Attached patch Patch for various test files (obsolete) — Splinter Review
Attachment #727628 - Flags: review?(jwalden+bmo)
Attached patch Patch for the rest of test files (obsolete) — Splinter Review
Attachment #727630 - Flags: review?(jwalden+bmo)
Attached patch Patch for various js* files (obsolete) — Splinter Review
Attachment #727631 - Flags: review?(jwalden+bmo)
Attached patch Patch for some other js* files (obsolete) — Splinter Review
Attachment #727632 - Flags: review?(jwalden+bmo)
Attached patch Patch for Library.cpp (obsolete) — Splinter Review
Attachment #727633 - Flags: review?(jwalden+bmo)
Attached patch Patch for Profilers.cpp (obsolete) — Splinter Review
Attachment #727634 - Flags: review?(jwalden+bmo)
Attachment #727635 - Flags: review?(jwalden+bmo)
Attached patch First patch for js.cpp (obsolete) — Splinter Review
Attachment #727641 - Flags: review?(jwalden+bmo)
Attached patch Second patch for js.cpp (obsolete) — Splinter Review
Attachment #727642 - Flags: review?(jwalden+bmo)
Attached patch Third patch for js.cpp (obsolete) — Splinter Review
Attachment #727643 - Flags: review?(jwalden+bmo)
Attached patch Forth patch for js.cpp (obsolete) — Splinter Review
Attachment #727644 - Flags: review?(jwalden+bmo)
Attached patch Fifth patch for js.cpp (obsolete) — Splinter Review
Attachment #727645 - Flags: review?(jwalden+bmo)
Attached patch Sixth patch for js.cpp (obsolete) — Splinter Review
Attachment #727646 - Flags: review?(jwalden+bmo)
Attached patch Seventh patch for js.cpp (obsolete) — Splinter Review
Attachment #727647 - Flags: review?(jwalden+bmo)
Attached patch Eighth patch for js.cpp (obsolete) — Splinter Review
Attachment #727648 - Flags: review?(jwalden+bmo)
Attached patch Ninth patch for js.cpp (obsolete) — Splinter Review
Attachment #727649 - Flags: review?(jwalden+bmo)
Attached patch Tenth patch for js.cpp (obsolete) — Splinter Review
Attachment #727650 - Flags: review?(jwalden+bmo)
Attached patch Eleventh patch for js.cpp (obsolete) — Splinter Review
Attachment #727651 - Flags: review?(jwalden+bmo)
Oops, just reread your comment. Somehow the number 200 stuck into my mind and I translated that into 200 lines per patch. Sorry for the long list of emails. I'll split CTypes.cpp's patch in two and upload them now.
Attached patch First patch for CTypes.cpp (obsolete) — Splinter Review
Attachment #727655 - Flags: review?(jwalden+bmo)
Attached patch Second patch for CTypes.cpp (obsolete) — Splinter Review
Attachment #727657 - Flags: review?(jwalden+bmo)
Regarding moving IsConstructing inside CallReceiver, according to :jorendorff, JSObject's implementation should not be made public, so I can't do it like how it's done in js/src/jsfuninlines.h. On the other hand, I can't do it like JS_IsConstructing, due to lack of a JSContext*.

Is there any other way to do this?
Flags: needinfo?(jwalden+bmo)
I'm fine with just changing the function not to take a cx argument. Or adding a JS_FRIEND_API function that doesn't take a cx argument.
Comment on attachment 727634 [details] [diff] [review]
Patch for Profilers.cpp

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

::: js/src/builtin/Profilers.cpp
@@ +230,4 @@
>          return JS_TRUE;
>      }
>  
> +    RequiredStringArg profileName(cx, args.length(), vp, 0, "startProfiling");

Still using vp for all RequiredStringArg users.
(In reply to Do Nhat Minh from comment #43)
> Regarding moving IsConstructing inside CallReceiver, according to
> :jorendorff, JSObject's implementation should not be made public, so I can't
> do it like how it's done in js/src/jsfuninlines.h.

The funky stuff there is only there to do a little bit of extra asserting.  I can't remember those assertions ever triggering.  I think we can just remove those problematic bits, and have isConstructing() just be |return vp.isMagic(JS_IS_CONSTRUCTING)| or its equivalent.  (Not sure why the magic-value was missing before -- we should add it, definitely, although we'll want tryservering to be sure the addition doesn't trigger any assertions.)
Flags: needinfo?(jwalden+bmo)
Attached patch New patch for Profilers.cpp (obsolete) — Splinter Review
Changed RequiredStringArg to using CallArgs
Attachment #727634 - Attachment is obsolete: true
Attachment #727634 - Flags: review?(jwalden+bmo)
Attachment #727967 - Flags: review?(Ms2ger)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #46)
> (In reply to Do Nhat Minh from comment #43)
> > Regarding moving IsConstructing inside CallReceiver, according to
> > :jorendorff, JSObject's implementation should not be made public, so I can't
> > do it like how it's done in js/src/jsfuninlines.h.
> 
> The funky stuff there is only there to do a little bit of extra asserting. 
> I can't remember those assertions ever triggering.  I think we can just
> remove those problematic bits, and have isConstructing() just be |return
> vp.isMagic(JS_IS_CONSTRUCTING)| or its equivalent.  (Not sure why the
> magic-value was missing before -- we should add it, definitely, although
> we'll want tryservering to be sure the addition doesn't trigger any
> assertions.)

I see. But what's tryservering?
Flags: needinfo?(jwalden+bmo)
Status: NEW → ASSIGNED
I'll remove IsConstructing from js/src/jsfuninlines.h

Should I remove JS_IsConstructing, too?
(In reply to Do Nhat Minh from comment #48)
> we should add it, definitely, although
> > we'll want tryservering to be sure the addition doesn't trigger any
> > assertions.)
> 
> I see. But what's tryservering?

Besides the main tree, Mozilla has a test tree.  If you push any change to it, it'll build the change on whatever set of platforms and with whatever set of build settings you want, and test it too.  Just something we do when we're uncertain whether a change is safe to make.  I'll do it this time around for the patches; in the longer run you can get commit access to the Try repository and test things yourself.

I'd leave JS_IsConstructing in for now.  If you read CallArgs.h you saw we have plans to change JSNative to take a const CallArgs&.  The time to remove JS_IsConstructing is probably then -- make a clean break of it.  In the meantime everything old still works, and people are free to use it (but recommended to stop doing so).
Flags: needinfo?(jwalden+bmo)
Okay. Can I change JS_IsConstructing to using CallReceiver::isConstructing?

When I replace `vp + 2` with `&callArgs[0]`, I get jit-test errors starting from test 107. Looking at the definition of PodCopy, I can't see why it's failing. Can you help clarify this?
Flags: needinfo?(jwalden+bmo)
Sorry, more info, I'm changing js/src/jsfun.cpp:1111
Change RequiredStringArg constructor to using const CallArgs& instead of CallArgs.

Obsoleting Patch V3 as the current patch diverges from it.
Attachment #727539 - Attachment is obsolete: true
Attachment #727967 - Attachment is obsolete: true
Attachment #727539 - Flags: review?(jwalden+bmo)
Attachment #727967 - Flags: review?(Ms2ger)
Attachment #728010 - Flags: review?(Ms2ger)
Comment on attachment 728010 [details] [diff] [review]
New new patch for Profilers.cpp

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

r=me

::: js/src/builtin/Profilers.cpp
@@ +206,1 @@
>              JS_ReportError(cx, "%s: not enough arguments", caller);

You don't have to address this, but I just noticed that every caller already makes sure this doesn't happen.

@@ +293,1 @@
>          ret = JS_DumpProfile(NULL, NULL);

If you want, you could also rewrite this function a bit to assign to args.rval() directly and return early, instead of keeping the ret local; that would be closer to the other functions here. (Either way is fine with me.)
Attachment #728010 - Flags: review?(Ms2ger) → review+
(In reply to :Ms2ger from comment #54)
> Comment on attachment 728010 [details] [diff] [review]
> New new patch for Profilers.cpp
> 
> Review of attachment 728010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: js/src/builtin/Profilers.cpp
> @@ +206,1 @@
> >              JS_ReportError(cx, "%s: not enough arguments", caller);
> 
> You don't have to address this, but I just noticed that every caller already
> makes sure this doesn't happen.

A function not checking its inputs feels wrong to me. Future code might not do that check before calling. So I guess I'll leave that as is.

> @@ +293,1 @@
> >          ret = JS_DumpProfile(NULL, NULL);
> 
> If you want, you could also rewrite this function a bit to assign to
> args.rval() directly and return early, instead of keeping the ret local;
> that would be closer to the other functions here. (Either way is fine with
> me.)

I'll leave this one as is, too.
My bad. C++'s references, which is what CallArgs::operator[] is returning, aren't supposed to be taken address of.
Flags: needinfo?(jwalden+bmo)
Attached patch First part of CTypes.cpp (obsolete) — Splinter Review
Attachment #728187 - Flags: review?(jwalden+bmo)
Attached patch Second part for CTypes.cpp (obsolete) — Splinter Review
Attachment #728188 - Flags: review?(jwalden+bmo)
Attached patch First part for js.cpp (obsolete) — Splinter Review
Attachment #727628 - Attachment is obsolete: true
Attachment #727630 - Attachment is obsolete: true
Attachment #727631 - Attachment is obsolete: true
Attachment #727632 - Attachment is obsolete: true
Attachment #727633 - Attachment is obsolete: true
Attachment #727635 - Attachment is obsolete: true
Attachment #727641 - Attachment is obsolete: true
Attachment #727642 - Attachment is obsolete: true
Attachment #727643 - Attachment is obsolete: true
Attachment #727644 - Attachment is obsolete: true
Attachment #727645 - Attachment is obsolete: true
Attachment #727646 - Attachment is obsolete: true
Attachment #727647 - Attachment is obsolete: true
Attachment #727648 - Attachment is obsolete: true
Attachment #727649 - Attachment is obsolete: true
Attachment #727650 - Attachment is obsolete: true
Attachment #727651 - Attachment is obsolete: true
Attachment #727655 - Attachment is obsolete: true
Attachment #727657 - Attachment is obsolete: true
Attachment #728010 - Attachment is obsolete: true
Attachment #727628 - Flags: review?(jwalden+bmo)
Attachment #727630 - Flags: review?(jwalden+bmo)
Attachment #727631 - Flags: review?(jwalden+bmo)
Attachment #727632 - Flags: review?(jwalden+bmo)
Attachment #727633 - Flags: review?(jwalden+bmo)
Attachment #727635 - Flags: review?(jwalden+bmo)
Attachment #727641 - Flags: review?(jwalden+bmo)
Attachment #727642 - Flags: review?(jwalden+bmo)
Attachment #727643 - Flags: review?(jwalden+bmo)
Attachment #727644 - Flags: review?(jwalden+bmo)
Attachment #727645 - Flags: review?(jwalden+bmo)
Attachment #727646 - Flags: review?(jwalden+bmo)
Attachment #727647 - Flags: review?(jwalden+bmo)
Attachment #727648 - Flags: review?(jwalden+bmo)
Attachment #727649 - Flags: review?(jwalden+bmo)
Attachment #727650 - Flags: review?(jwalden+bmo)
Attachment #727651 - Flags: review?(jwalden+bmo)
Attachment #727655 - Flags: review?(jwalden+bmo)
Attachment #727657 - Flags: review?(jwalden+bmo)
Attachment #728189 - Flags: review?(jwalden+bmo)
Attached patch Second part for js.cpp (obsolete) — Splinter Review
Attachment #728190 - Flags: review?(jwalden+bmo)
Attached patch First patch for various files (obsolete) — Splinter Review
Attachment #728191 - Flags: review?(jwalden+bmo)
Attachment #728010 - Attachment is obsolete: false
Attached patch Second patch for various files (obsolete) — Splinter Review
Attachment #728192 - Flags: review?(jwalden+bmo)
Sorry, I accidentally obsoleted that. It survives the merge.
The newly uploaded patches are the ones that are merged with origin/master at 110f758308d68...

They also move IsConstructing in js/src/jsfuninlines.cpp into CallReceiver in js/public/CallArgs.h, change JS_IsConstructing to using CallReceiver::isConstructing and fix all code to call CallReceiver::isConstructing instead of the other two.
Comment on attachment 728187 [details] [diff] [review]
First part of CTypes.cpp

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

I looked at about a third or so of this, and it's mostly on the right track.  I'm a little surprised you're getting rid of *_TO_JSVAL and JSVAL_TO_* in a few places, even beyond the ones where the value fed directly into an rval().  But there's nothing wrong with making such improvements (although be careful about putting too many distinct things in one patch -- generally fixing one thing at a time in a patch is best).

However, I saw a bunch of use of val.toObject() (which returns a JSObject&) in contexts where a JSObject* was required -- you're missing & in a bunch of places.  I'm happy to review patches that compile, but I don't have enough time to help out with patches that don't.  :-(  Could you put a little work into compiling these, and fixing any errors that manifest?  Thanks!

::: js/src/ctypes/CTypes.cpp
@@ +3081,5 @@
>  
>  JSBool
>  CType::ConstructData(JSContext* cx,
>                       unsigned argc,
>                       jsval* vp)

We'd like to get rid of all uses of argc/vp, to transition to CallArgs.  This method has more uses of argc/vp below, which should be converted to pass const CallArgs& parameters.  Nothing wrong with the changes here, just that they're not quite complete changes.  :-)

I'd suggest taking a look back when you're done and finding all the places where argc/vp are still used, outside of direct calls to CallArgsFromVp.  I think we want to land this patch and get that incremental gain, then have other fixes in separate patches.  No good reason to hold up the entire thing because of incompleteness, seems to me.

@@ +3742,5 @@
>  {
>    JS_ASSERT(CType::IsCType(obj));
>  
>    jsval slot = JS_GetReservedSlot(obj, SLOT_PROTO);
> +  JSObject* prototype = slot.toObject();

Hmm, that's not going to compile -- needs a &.

@@ +3752,3 @@
>      return JS_TRUE;
>  
> +  JSObject* proto = v.toObject();

Nor is this!

@@ +3781,3 @@
>  
> +  JS_ASSERT(!valCTypes.isPrimitive());
> +  return valCTypes.toObject();

Heh, those assertions are pretty awesome.  You're missing a & here as well.

@@ +3851,2 @@
>    RootedObject obj(cx);
> +  if (arg.isPrimitive() || !CType::IsCType(obj = arg.toObject())) {

More & missing.  This looks like something you'll want to fix everywhere.  Have you managed to compile a Firefox build?  You should definitely invest some time to learn how to do that, before you try doing too much here.  #introduction on IRC has people who can help with this, if you catch it at a time when people are around to help.

@@ +3866,5 @@
>  PointerType::CreateInternal(JSContext* cx, HandleObject baseType)
>  {
>    // check if we have a cached PointerType on our base CType.
>    jsval slot = JS_GetReservedSlot(baseType, SLOT_PTR);
> +  if (!slot.isVoid())

slot.isUndefined(); we used to call |undefined| "void" in APIs, but it's much clearer making it "undefined" now, as that's the direction specs went.

@@ +3966,2 @@
>        thisObj = NULL;
> +    } else if (!args[1].isPrimitive()) {

This is probably better written as |args[1].isObject()|.  There used to be a JSVAL_IS_OBJECT, that would return true for either an object *or* null.  JSVAL_IS_PRIMITIVE was introduced to provide what most people really meant, and it propagated pretty much everywhere.  Then we ended up removing JSVAL_IS_OBJECT completely to eliminate the issue.  Now that we have methods on Value that actually behave the way you'd expect, unless you're thinking in terms of primitiveness it's best to use isObject()/toObject() directly.
Attachment #728187 - Flags: review?(jwalden+bmo)
I don't know why but on my machine, `make` does not recompile when I change js/src/ctypes/CTypes.cpp, which is why I don't see compilation errors on those. I'm using ArchLinux X64.
I just fixed some of the errors in js/src/ctypes/CTypes.cpp and redid a `make clean && make -sj2`. The compilation seems not to include that file at all. Can you give me any clue on this?
Flags: needinfo?(jwalden+bmo)
it turns out I need to enable ctypes when configure the project, I.e. /path/to/configure --enable-debug --disable-optimize --enable-ctypes
Flags: needinfo?(jwalden+bmo)
Attachment #728187 - Attachment is obsolete: true
Attachment #728726 - Flags: review?(jwalden+bmo)
Attachment #728188 - Attachment is obsolete: true
Attachment #728188 - Flags: review?(jwalden+bmo)
Attachment #728727 - Flags: review?(jwalden+bmo)
Attachment #728189 - Attachment is obsolete: true
Attachment #728189 - Flags: review?(jwalden+bmo)
Attachment #728728 - Flags: review?(jwalden+bmo)
Attached patch Second patch for js.cpp (obsolete) — Splinter Review
Attachment #728190 - Attachment is obsolete: true
Attachment #728190 - Flags: review?(jwalden+bmo)
Attachment #728729 - Flags: review?(jwalden+bmo)
Attached patch First patch for various files (obsolete) — Splinter Review
Attachment #728191 - Attachment is obsolete: true
Attachment #728191 - Flags: review?(jwalden+bmo)
Attachment #728730 - Flags: review?(jwalden+bmo)
Attached patch Second patch for various files (obsolete) — Splinter Review
Attachment #728192 - Attachment is obsolete: true
Attachment #728192 - Flags: review?(jwalden+bmo)
Attachment #728731 - Flags: review?(jwalden+bmo)
The new batch of patches fixes the problems in js/src/ctypes/CTypes.cpp and revert the use of Value::isPrimitive back to JSVAL_IS_PRIMITIVE
@Waldo do you have any comments for these patches?
Comment on attachment 728726 [details] [diff] [review]
First patch for CTypes.cpp

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

Sorry for the delay -- I have a lot of different patches to juggle and review, amidst my own code to write, so I'm not always going to get to patches within a day or so -- more likely from a few days up to a week or a little longer.  Other people besides me can review patches, too, if you want to split the load across people.  :-)  :Ms2ger, for example, can review these patches.  (Doubtless Ms2ger is now going to look for something to throw at me.  ;-) )

As a general comment, you should put a summary in your patches and associate your name/email address with them, perhaps like this:

Bug 845478 - Use CallArgs to access argc/vp in most places in half of CTypes.cpp.  r=jwalden

qrefresh and qnew have a -m argument for specifying the message on the commandline, and -e for opening up an editor to set it.  The user name can be set via -u "...".  But if it's your name, setting the username in ~/.hgrc means you can use -U to add it.  Personally, I have this in my ~/.hgrc so I never have to add it manually for my own patches:

[defaults]
qnew = -U

::: js/src/ctypes/CTypes.cpp
@@ +4083,4 @@
>                              JSHandleId idval,
>                              MutableHandleValue vp)
>  {
> +  CallReceiver receiver = CallReceiverFromVp(vp.address());

(JSContext *, HandleObject, HandleId, MutableHandleValue) methods -- that is, JSPropertyOp -- don't follow the CallArgs convention at all.  There *is* a plan to convert these to JSNative in JSPropertySpec arrays, but that's definitely a separate bug/issue.  So you shouldn't add CallArgs uses (CallReceiver here -- there's a fair chance we'll integrate CallReceiver into CallArgs at some point, to have only one class here) to methods like this right now.

Given that I only see one other use of CallReceiver in this file (which does want to be CallArgs, but not this way -- rather by making the two callers create the CallArgs and pass it down to the helper method), and I can fix that one and this one pretty easily, I'll stamp this with an r+ and queue it up to land when I have time (probably within a day, but no guarantees).

For the other patches, tho, if there's more than a couple cases like this, I'll probably throw the patch back at you to correct.  :-)
Attachment #728726 - Flags: review?(jwalden+bmo) → review+
Thanks, I will do that the next time around.
First bit of ctypes changing landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c42018a4754
Whiteboard: [lang=c++][mentor=jwalden] → [lang=c++][mentor=jwalden][leave open]
cool! Thanks for seeing it through.

about the patch for Profilers.CPP, can you land it in the main tree, too?

I've emailed :Ms2ger for help with reviewing but he/she hasn't replied.
I'm going to play my "I'm not a peer" card here... I bet evilpie could do it.
Attachment #728731 - Flags: review?(jwalden+bmo) → review?(evilpies)
Attachment #728730 - Flags: review?(jwalden+bmo) → review?(evilpies)
Attachment #728729 - Flags: review?(jwalden+bmo) → review?(evilpies)
Attachment #728010 - Flags: checkin?(Ms2ger)
Comment on attachment 728729 [details] [diff] [review]
Second patch for js.cpp

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

Good effort here. This code still has tons of C-style stuff and is kind of weird. There are a lot of smaller things, you probably didn't know about, so I am giving you a r-. 

The most obvious pattern is args.length() < 1 ? UndefinedValue() : args[0], can be much cleaner written as args.get(0).
There are some other issues, but I didn't bother you with them, because they are per-existing.

::: js/src/shell/js.cpp
@@ +2342,4 @@
>      JSPropertyDesc *pd;
>      jsval v;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

Try moving at the top of the function.

@@ +2343,5 @@
>      jsval v;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (!JS_ValueToObject(cx, args.length() == 0 ? UndefinedValue() : args[0], vobj.address()))

This can be args.get(0)

@@ +2393,3 @@
>      RootedScript script(cx);
>  
> +    script = ValueToScript(cx, args.length() == 0 ? UndefinedValue() : args[0]);

.get(0) as well.

@@ +2406,3 @@
>      int32_t i;
>  
> +    if (!JS_ValueToInt32(cx, args.length() == 0 ? UndefinedValue() : args[0], &i))

get(0)

@@ +2630,2 @@
>      JS::Value v;
> +    if (args.length() < 1 || !((v = args[0]).isObject())) {

Remove the JS::Value v;
Make this if (!args.get(0).isObject())

@@ +2632,4 @@
>          JS_ReportError(cx, "shapeOf: object expected");
>          return false;
>      }
>      JSObject *obj = &v.toObject();

&args[0].toObject

@@ +2632,5 @@
>          JS_ReportError(cx, "shapeOf: object expected");
>          return false;
>      }
>      JSObject *obj = &v.toObject();
> +    args.rval().setDouble((double) ((uintptr_t)obj->lastProperty() >> 3));

C++ style cast
double(uintptr_r(obj->lastProperty()) >> 3)

@@ +2776,4 @@
>  {
>      int64_t t_ticks;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top

@@ +2781,5 @@
>          t_ticks = 0;
>      } else {
>          double t_secs;
>  
> +        if (!JS_ValueToNumber(cx, args.length() == 0 ? UndefinedValue() : args[0], &t_secs))

get()

@@ +3085,5 @@
>          JS_ReportError(cx, "Wrong number of arguments");
>          return false;
>      }
>  
> +    jsval v = args[0];

Value

@@ +3090,1 @@
>      if (JSVAL_IS_PRIMITIVE(v)) {

isPrimitive()

@@ +3112,5 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
>                               "compile", "0", "s");
>          return false;
>      }
> +    jsval arg0 = args[0];

Value

@@ +3390,1 @@
>      if (JSVAL_IS_PRIMITIVE(v)) {

Value v = args.get(0);
if (v.isPrimitive()) {

@@ +3432,5 @@
>  
>  static JSBool
>  Serialize(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);

RootedValue v(cx, args.get(0))

@@ +3456,5 @@
>  static JSBool
>  Deserialize(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    Rooted<jsval> v(cx, args.length() > 0 ? args[0] : UndefinedValue());

RootedValue v(cx, args.get(0))

@@ +3980,4 @@
>  {
>      fprintf(gOutFile, "%s\n", JS_GetImplementationVersion());
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top.

@@ +4000,5 @@
>              if (!PrintHelp(cx, obj))
>                  return false;
>          }
>      } else {
> +        CallArgs args = CallArgsFromVp(argc, vp);

You already have args at the top.

@@ +4002,5 @@
>          }
>      } else {
> +        CallArgs args = CallArgsFromVp(argc, vp);
> +        for (unsigned i = 0; i < args.length(); i++) {
> +            if (JSVAL_IS_PRIMITIVE(args[i])) {

args[0].isPrimitive()

@@ +4233,1 @@
>      JSObject *obj = JS_THIS_OBJECT(cx, vp);

if (!args.thisv().isObject()) {
    args.rval.setUndefined()
    return true;
}
JSObject *obj = &args.thisv.toObject();

@@ +4233,2 @@
>      JSObject *obj = JS_THIS_OBJECT(cx, vp);
>      if (!obj)

Can obviously go away after that.

@@ +4250,1 @@
>      JSObject *obj = JS_THIS_OBJECT(cx, vp);

Same here.

@@ +4299,5 @@
>      bool ok;
>      pid_t pid;
>      int status;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top

@@ +4306,2 @@
>  
>      fun = JS_ValueToFunction(cx, vp[0]);

I am not sure what this code is even doing here. Don't worry about it.

@@ +4667,1 @@
>      RootedObject obj(cx, JS_THIS_OBJECT(cx, vp));

Like a few above.

if (!args.thisv().isObject())
...
return

RootedObject obj(cx, &args.thisv())

@@ +4688,1 @@
>      RootedObject obj(cx, JS_THIS_OBJECT(cx, vp));

again
Attachment #728729 - Flags: review?(evilpies) → review-
Comment on attachment 728730 [details] [diff] [review]
First patch for various files

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

Looks quite good as well. Changing this to feedback+. Please attach a new patch. Maybe next time don't do such massive changes at once, without previous feedback.

::: js/public/CallArgs.h
@@ +177,5 @@
> +
> +  public:
> +    /*
> +     * If isConstructing() is true, thisv() must not be used; the constructor
> +     * should construct and return a new object. Otherwise, the native is

nit: "the constructor should construct" sounds strange.

@@ +180,5 @@
> +     * If isConstructing() is true, thisv() must not be used; the constructor
> +     * should construct and return a new object. Otherwise, the native is
> +     * called as an ordinary function and thisv() may be used.
> +     */
> +    bool isConstructing() const {

Nice change!

::: js/src/builtin/TestingFunctions.cpp
@@ +190,4 @@
>       * object, we collect the object's compartment (and any other compartments
>       * scheduled for GC). Otherwise, we collect all compartments.
>       */
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top.

@@ +252,1 @@
>      }

All this can be something like:
JS::RootedString str(cx, JS_ValueToString(args.get(0)));
if (!str)
   return false;

@@ +375,5 @@
>          RootedObject callee(cx, &args.callee());
>          ReportUsageError(cx, callee, "Too many arguments");
>          return JS_FALSE;
>      }
> +    if (!JS_ValueToECMAUint32(cx, args.length() < 1 ? JSVAL_VOID : args[0], &zeal))

args.get(0)

@@ +422,3 @@
>  
> +    for (unsigned i = 0; i < args.length(); i++) {
> +        Value arg(args[i]);

Just for consistency (totally not caring about the potential rooting problems):
Value arg = args[i];

@@ +437,4 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    if (args.length()) {

> 0

@@ +450,5 @@
>  static JSBool
>  VerifyPostBarriers(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length()) {

> 0

@@ +666,4 @@
>      JSCountHeapNode *node;
>      size_t counter;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

Top

@@ +818,4 @@
>  
>      fclose(dumpFile);
>  
> +    args.rval().set(JSVAL_VOID);

setUndefined()

@@ +885,4 @@
>      cx->runtime->spsProfiler.enableSlowAssertions(args[0].toBoolean());
>      cx->runtime->spsProfiler.enable(true);
>  
> +    args.rval().set(JSVAL_VOID);

setUndefined()

::: js/src/ctypes/Library.cpp
@@ +203,4 @@
>      return JS_FALSE;
>    }
>  
> +  if (args.length() != 1 || args[0].isUndefined()) {

if (!args.hasDefined(0))

@@ +223,1 @@
>    JSObject* obj = JS_THIS_OBJECT(cx, vp);

I had this in the other patch as well. Please check if you could use thisv() here.

@@ +237,4 @@
>    UnloadLibrary(obj);
>    JS_SetReservedSlot(obj, SLOT_LIBRARY, PRIVATE_TO_JSVAL(NULL));
>  
> +  args.rval().set(JSVAL_VOID);

setUndefined

@@ +248,1 @@
>    RootedObject obj(cx, JS_THIS_OBJECT(cx, vp));

thisv() maybe.

@@ +296,4 @@
>        return JS_FALSE;
>    } else {
>      // Case 2).
> +    if (JSVAL_IS_PRIMITIVE(args[1]) ||

args[1].isPrimitive()

::: js/src/jsapi-tests/testDebugger.cpp
@@ +268,5 @@
>          JS_ASSERT(script);
>  
>          if (!JS_SetSingleStepMode(cx, script, true))
>              return false;
> +        args.rval().set(JSVAL_VOID);

setUndefined
Attachment #728730 - Flags: review?(evilpies) → feedback+
Comment on attachment 728731 [details] [diff] [review]
Second patch for various files

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

Sorry I wont get to do this until I am back around Friday.

::: js/src/jsbool.cpp
@@ +136,3 @@
>          JSObject *obj = BooleanObject::create(cx, b);
>          if (!obj)
> +            return JS_FALSE;

no.
Attachment #728010 - Flags: checkin?(Ms2ger)
Comment on attachment 728728 [details] [diff] [review]
First patch for js.cpp

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

This is pretty much reasonable.  I've only been skimming the other comments, but I did see evilpie say we want CallArgs at the very top of functions.  He's definitely right about that.  (This makes it easier to remove them, when we get around to changing JSNative to take |const CallArgs&|.)  Also -- looking at PrintInternal specifically -- we want the callers of utility functions like that (the actual JSNatives, that is) to create the CallArgs, then pass that to the utility functions underneath.

The smattering of other jsval->Value changes are okay and all, although you were a bit inconsistent in making them, to be sure.  Forward progress beats no progress, tho, so I'll take it as-is.

Anyway, I fixed up the nits I saw and will push the adjusted patch.  Thanks!  I'm multitasking on reviews, so possibly you might be able to fix up the other patches accordingly before I can get to the others?
Attachment #728728 - Flags: review?(jwalden+bmo) → review+
(In reply to Tom Schuster [:evilpie] from comment #83)
> @@ +4233,1 @@
> >      JSObject *obj = JS_THIS_OBJECT(cx, vp);
> 
> if (!args.thisv().isObject()) {
>     args.rval.setUndefined()
>     return true;
> }
> JSObject *obj = &args.thisv.toObject();

Could you tell me why you return true in this case while the old code returns false?
Attachment #728726 - Flags: checkin?(ryanvm)
Attachment #728728 - Flags: checkin?(ryanvm)
Attachment #728726 - Flags: checkin?(ryanvm)
Attachment #728728 - Flags: checkin?(ryanvm)
Sorry I made a mistake asking for a checkin again.
Attached patch Second patch for js.cpp (obsolete) — Splinter Review
Attachment #728729 - Attachment is obsolete: true
Attachment #732357 - Flags: review?(evilpies)
Attached patch First patch for various files (obsolete) — Splinter Review
Attachment #728730 - Attachment is obsolete: true
Attachment #732359 - Flags: review?(evilpies)
Comment on attachment 728727 [details] [diff] [review]
Second patch for CTypes.cpp

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

I'll make the changes noted here and push this patch, too, along with the other one I have in my queue waiting for the tree to reopen.  Thanks!

::: js/src/ctypes/CTypes.cpp
@@ +7442,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToString(cx, obj, args.length(), vp, false);

Here's another place where you really should be passing along |args|, and not two separate arguments.

@@ +7457,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToSource(cx, obj, args.length(), vp, false);

And here too.

@@ +7612,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToString(cx, obj, args.length(), vp, true);

And here.

@@ +7627,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToSource(cx, obj, args.length(), vp, true);

And here.
Attachment #728727 - Flags: review?(jwalden+bmo) → review+
Would you mind to attach the patches again, so that they work with [Review]. I think the problem is the From: line at the very top.
Comment on attachment 732359 [details] [diff] [review]
First patch for various files

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

Looks very good overall. Thank you!

::: js/src/ctypes/Library.cpp
@@ +194,5 @@
>  JSBool
>  Library::Open(JSContext* cx, unsigned argc, jsval *vp)
>  {
> +  CallArgs args = CallArgsFromVp(argc, vp);
> +  JSObject* ctypesObj = &args.thisv().toObject();

This is now going to crash when .thisv() is not an object. Just revert this change, we will take care of JS_THIS_OBJECT globally.

@@ +219,5 @@
>  JSBool
>  Library::Close(JSContext* cx, unsigned argc, jsval* vp)
>  {
> +  CallArgs args = CallArgsFromVp(argc, vp);
> +  JSObject* obj = &args.thisv().toObject();

Same here. I am sorry.
Attachment #732359 - Flags: review?(evilpies) → review+
Comment on attachment 732357 [details] [diff] [review]
Second patch for js.cpp

Please fix this one. I now think the missing file path is at fault. Are you manually altering patches?
Attachment #732357 - Flags: review?(evilpies)
Comment on attachment 728731 [details] [diff] [review]
Second patch for various files

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

Nice work!

::: js/src/jsapi-tests/tests.h
@@ +254,4 @@
>  
>          putchar('\n');
>          fflush(stdout);
> +        args.rval().set(JSVAL_VOID);

setUndefined()

::: js/src/jsapi.h
@@ -4869,5 @@
>  static JS_ALWAYS_INLINE JSBool
>  JS_IsConstructing(JSContext *cx, const jsval *vp)
>  {
> -#ifdef DEBUG
> -    JSObject *callee = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));

I don't think it's acceptable to just lose these assertions. I think I remember you saying that not every call to the new isConstructing() has a cx? I think the best solution is to just update these assertions to use args.callee() and leave them here.

::: js/src/jsbool.cpp
@@ +136,3 @@
>          JSObject *obj = BooleanObject::create(cx, b);
>          if (!obj)
> +            return JS_FALSE;

Please revert this change, internally we avoid JS_FALSE and JS_TRUE whenever we can.

::: js/src/jsfun.cpp
@@ +1111,3 @@
>          return false;
>  
> +    args.setCallee(invokedArgs.rval());

vp[0] has a slight double meaning. On function entry vp[0] contains the callee, but after leaving the function vp[0] is used to determine the return value. So this should be args.rval().set(invokedArgs.rval()).

::: js/src/jsfuninlines.h
@@ -119,5 @@
> -static JS_ALWAYS_INLINE bool
> -IsConstructing(const Value *vp)
> -{
> -#ifdef DEBUG
> -    JSObject *callee = &JS_CALLEE(cx, vp).toObject();

How did that even work?

::: js/src/jsnum.cpp
@@ +424,1 @@
>  

I think you can rewrite this like this:

double number = 0;
if (args.length() > 0) {
  if (!ToNumber(cx, args[0], &number))
    return false;
}

@@ +424,4 @@
>  
> +    if (args.length() > 0) {
> +        if (!ToNumber(cx, &args[0]))
> +            return JS_FALSE;

false

@@ +431,4 @@
>      }
>  
>      if (!isConstructing)
> +        return JS_TRUE;

true! Also fix all the other cases, please.

@@ +435,2 @@
>  
> +    JSObject *obj = NumberObject::create(cx, args.calleev().toNumber());

This could just be
JSObject *obj = NumberObject::create(cx, number);

@@ +437,3 @@
>      if (!obj)
> +        return JS_FALSE;
> +    args.setCallee(ObjectValue(*obj));

args.rval().setObject(*obj)

::: js/src/jsreflect.cpp
@@ +2964,4 @@
>  
>      RootedObject builder(cx);
>  
> +    RootedValue arg(cx, args.length() > 1 ? args[1] : UndefinedValue());

args.get(1)

::: js/src/perf/jsperf.cpp
@@ +167,5 @@
>  pm_construct(JSContext* cx, unsigned argc, jsval* vp)
>  {
>      uint32_t mask;
> +
> +    JS::CallArgs args = CallArgsFromVp(argc, vp);

Top please.
Attachment #728731 - Flags: review?(evilpies) → review+
Hey Do! Are you still working on this?
Depends on: 984112
Assigning myself as Do does not seem to be working on this any longer.
Assignee: mrordinaire → birunthan
In this and subsequent patches, I also changed JSVAL_IS_*/JSVAL_TO_* to the JS::Value equivalents in lines that were touched for other reasons. The rest will presumably be converted in bug 952650.
Attachment #728731 - Attachment is obsolete: true
Attachment #732357 - Attachment is obsolete: true
Attachment #732359 - Attachment is obsolete: true
Attachment #8392282 - Flags: review?(evilpies)
Attachment #8392286 - Flags: review?(evilpies)
Comment on attachment 8392284 [details] [diff] [review]
Part 3: Use JS::CallArgs instead of `argc` in js/src/

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

::: js/src/json.cpp
@@ +820,4 @@
>  
>      /* Steps 2-5. */
>      return ParseJSONWithReviver(cx, ConstTwoByteChars(flat->chars(), flat->length()),
>                                  flat->length(), reviver, args.rval());

Couldn't you just use args.get(1) here now?

@@ +831,1 @@
>      RootedObject replacer(cx, (argc >= 2 && vp[3].isObject())

I think you missed this one.
Attachment #8392284 - Flags: review?(evilpies) → review+
Comment on attachment 8392285 [details] [diff] [review]
Part 4: Use JS::CallArgs instead of `argc` in js/src/builtin/

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

::: js/src/builtin/TestingFunctions.cpp
@@ +527,5 @@
>  static bool
>  VerifyPostBarriers(JSContext *cx, unsigned argc, jsval *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() != 0) {

let's stay with args.length()

@@ +542,5 @@
>  GCState(JSContext *cx, unsigned argc, jsval *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    if (args.length() != 0) {

same
Attachment #8392285 - Flags: review?(evilpies) → review+
Comment on attachment 8392286 [details] [diff] [review]
Part 5: Use JS::CallArgs instead of `vp` in js/src/

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

::: js/src/builtin/TestingFunctions.cpp
@@ +829,5 @@
>          JS_ReportOutOfMemory(cx);
>          return false;
>      }
>  
> +    args.rval().set(JS_NumberValue(double(counter)));

I am not sure what counter is, but you probably want something like .setNumber()

@@ +1017,5 @@
>      }
>  
>      JSFunction *fun = &args[0].toObject().as<JSFunction>();
>      JSString *str = fun->displayAtom();
> +    args.rval().setString(str == nullptr ? cx->runtime()->emptyString : str);

Can you make this str ? str : cx->runtime()->emptyStirng
Attachment #8392286 - Flags: review?(evilpies) → review+
Comment on attachment 8392290 [details] [diff] [review]
Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL}

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

Great work \o/
Attachment #8392290 - Flags: review?(evilpies) → review+
Comment on attachment 8392283 [details] [diff] [review]
Part 2: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/

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

::: js/src/ctypes/Library.cpp
@@ +207,5 @@
>      JS_ReportError(cx, "not a ctypes object");
>      return false;
>    }
>  
> +  if (args.length() != 1 || args[0].isUndefined()) {

!args.hasDefined(0)
Attachment #8392283 - Flags: review?(evilpies) → review+
Comment on attachment 8392282 [details] [diff] [review]
Part 1: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/shell/

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

::: js/src/shell/js.cpp
@@ +3383,5 @@
>          double d = 0.0;
>          JSShellContextData *data = GetContextData(cx);
>          if (data)
>              d = PRMJ_Now() - data->startTime;
> +        args.rval().set(JS_NumberValue(d));

setDouble

@@ +4110,1 @@
>      if (JSVAL_IS_PRIMITIVE(v)) {

v.isPrimitive()
Attachment #8392282 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #114)
> Comment on attachment 8392290 [details] [diff] [review]
> Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL}
> 
> Review of attachment 8392290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work \o/

We should probably announce this to some newsgroup.
Keywords: dev-doc-needed
Also https://developer.mozilla.org/en-US/docs/SpiderMonkey/31 will need updating for this.
Comment on attachment 8392289 [details] [diff] [review]
Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

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

Smallish nitpicks, and some slight refactoring I'd prefer you do to fix one of the problems here more thoroughly.  Otherwise in fair shape, second patch for review should be good.

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +196,5 @@
>  static bool
>  BuildDate(JSContext *cx, unsigned argc, JS::Value *vp)
>  {
>      fprintf(stdout, "built on %s at %s\n", __DATE__, __TIME__);
>      return true;

THis method needs |JS::CallArgs args = JS::CallArgsFromVp(argc, vp);| at the start and a |args.rval().setUndefined();| just before |return true| at the end.  (Right now this method returns itself by happenstance, which eventually we're going to make trigger an assertion.)

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +781,5 @@
>  {
>    NetAddr remoteAddress;
>    nsAutoCString localDottedDecimal;
>    JSContext *cx = mJSRuntime->Context();
> +  JS::CallReceiver call = CallReceiverFromVp(vp);

This kind of works, but not well.  The issue here is this code is old and works in terms of |vp| at all.  Instead, it should be passed a |const JS::CallArgs&| by the caller.

So, then, PACMyIpAddress should have a |JS::CallArgs args = JS::CallArgsFromVp(argc, vp);| and should then pass |args| to ProxyAutoConfig::MyIPAddress.  That, then, should pass |args| into this code.

@@ +822,5 @@
>    if (dns && NS_SUCCEEDED(dns->GetMyHostName(hostName)) &&
>        PACResolveToString(hostName, localDottedDecimal, kTimeout)) {
>      JSString *dottedDecimalString =
>        JS_NewStringCopyZ(cx, localDottedDecimal.get());
> +    call.rval().setString(dottedDecimalString);

Hmm.  So we fail to handle a JS OOM correctly here, then.  Could you file a followup bug to fix this?  Just need to make this method return true/false to indicate success/failure, and indicate got-an-IP-address status through an outparam.  Should be easy to fix.

@@ +841,5 @@
>    // who knows? let's fallback to localhost
>    localDottedDecimal.AssignLiteral("127.0.0.1");
>    JSString *dottedDecimalString =
>      JS_NewStringCopyZ(cx, localDottedDecimal.get());
> +  call.rval().setString(dottedDecimalString);

Same failure to handle OOM here.  This one's easy to fix, tho, just needs |if (!dottedDecimalString) return false;|.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +899,5 @@
>    case REFLECT_CORRUPT:
>      JS_ReportError(cx, "Histogram is corrupt");
>      return false;
>    case REFLECT_OK:
> +    args.rval().setObjectOrNull(snapshot);

If you look further up, |snapshot| was only null if an operation failed.  So it's non-null here, and you should instead use |args.rval().setObject(*snapshot);|.
Attachment #8392289 - Flags: review?(jwalden+bmo) → feedback+
Comment on attachment 8392287 [details] [diff] [review]
Part 6: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...}, `argc`, `vp` in js/xpconnect/

r=me
Attachment #8392287 - Flags: review?(bzbarsky) → review+
Attachment #8392288 - Flags: review?(vyang) → review+
Blocks: 986076
Comment 119 addressed. Bug 986076 filed for the JS OOM issue.
Attachment #8392289 - Attachment is obsolete: true
Attachment #8395015 - Flags: review?(jwalden+bmo)
Attachment #8392290 - Attachment is obsolete: true
Attachment #8395016 - Flags: review+
Keywords: checkin-needed
Whiteboard: [lang=c++][mentor=jwalden][leave open] → [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 8]
Whiteboard: [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 8] → [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 7]
Attachment #728726 - Flags: checkin+
Attachment #728727 - Flags: checkin+
Attachment #728728 - Flags: checkin+
Comment on attachment 8395006 [details] [diff] [review]
Part 1: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/shell/

https://hg.mozilla.org/integration/mozilla-inbound/rev/92d2569932a4
Attachment #8395006 - Flags: checkin+
Comment on attachment 8395007 [details] [diff] [review]
Part 2: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e8f128c7a8
Attachment #8395007 - Flags: checkin+
Comment on attachment 8395009 [details] [diff] [review]
Part 4: Use JS::CallArgs instead of `argc` in js/src/builtin/

https://hg.mozilla.org/integration/mozilla-inbound/rev/595c8c60d676
Attachment #8395009 - Flags: checkin+
Comment on attachment 8395012 [details] [diff] [review]
Part 6: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...}, `argc`, `vp` in js/xpconnect/

https://hg.mozilla.org/integration/mozilla-inbound/rev/58e088726ab7
Attachment #8395012 - Flags: checkin+
Comment on attachment 8395013 [details] [diff] [review]
Part 7: Use JS::CallArgs instead of JS_ARGV, `argc` in ipc/{ril,nfc}/

https://hg.mozilla.org/integration/mozilla-inbound/rev/a43111585107
Attachment #8395013 - Flags: checkin+
Keywords: checkin-needed
Whiteboard: [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 7] → [lang=c++][mentor=jwalden][leave open]
Comment on attachment 8395015 [details] [diff] [review]
Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

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

\o/
Attachment #8395015 - Flags: review?(jwalden+bmo) → review+
Keywords: checkin-needed
Comment on attachment 8395015 [details] [diff] [review]
Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

https://hg.mozilla.org/integration/mozilla-inbound/rev/e53e37d2c2bf
Attachment #8395015 - Flags: checkin+
Keywords: checkin-needed
Whiteboard: [lang=c++][mentor=jwalden][leave open] → [lang=c++][mentor=jwalden]
Blocks: 988025
https://hg.mozilla.org/mozilla-central/rev/e53e37d2c2bf
https://hg.mozilla.org/mozilla-central/rev/726e81b3c1a9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.