Closed
Bug 845478
Opened 11 years ago
Closed 10 years ago
JS shell should use JS::CallArgs instead of manual argc/vp+JS_SET_RVAL/JS_ARGV/etc.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: Waldo, Assigned: poiru)
References
Details
(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. :-)
Comment 1•11 years ago
|
||
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
Reporter | ||
Comment 2•11 years ago
|
||
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?
Comment 3•11 years ago
|
||
Jeff, thanks a lot for your detailed comment! Looks good enough for me to get started on this.
Comment 4•11 years ago
|
||
(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);
Reporter | ||
Comment 5•11 years ago
|
||
Er, yes. :-)
Reporter | ||
Comment 6•11 years ago
|
||
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.
Comment 7•11 years ago
|
||
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!
Comment 8•11 years ago
|
||
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
Reporter | ||
Comment 9•11 years ago
|
||
Try running this in js/src, as my best guess at a fix, then retry the other build steps: find . -name *.pyc | xargs rm
Comment 10•11 years ago
|
||
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 :)
Comment 11•11 years ago
|
||
> 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).
Comment 12•11 years ago
|
||
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
Updated•11 years ago
|
Status: ASSIGNED → NEW
Comment 13•11 years ago
|
||
hi, my name is Do Nhat Minh. I'm interested in fixing this bug. Can I get assigned to this bug?
Updated•11 years ago
|
Assignee: general → mrordinaire
Reporter | ||
Comment 14•11 years ago
|
||
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.
Comment 15•11 years ago
|
||
I will. Thanks.
Comment 16•11 years ago
|
||
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
Comment 17•11 years ago
|
||
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
Comment 18•11 years ago
|
||
This patch revert back to using JS_THIS_OBJECT, which is in the requirement.
Attachment #727526 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #727539 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 19•11 years ago
|
||
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.
Reporter | ||
Comment 20•11 years ago
|
||
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.
Comment 21•11 years ago
|
||
Thanks for the kind comments. I will get to it when I get back to my place.
Comment 22•11 years ago
|
||
Attachment #727628 -
Flags: review?(jwalden+bmo)
Comment 23•11 years ago
|
||
Attachment #727630 -
Flags: review?(jwalden+bmo)
Comment 24•11 years ago
|
||
Attachment #727631 -
Flags: review?(jwalden+bmo)
Comment 25•11 years ago
|
||
Attachment #727632 -
Flags: review?(jwalden+bmo)
Comment 26•11 years ago
|
||
Attachment #727633 -
Flags: review?(jwalden+bmo)
Comment 27•11 years ago
|
||
Attachment #727634 -
Flags: review?(jwalden+bmo)
Comment 28•11 years ago
|
||
Attachment #727635 -
Flags: review?(jwalden+bmo)
Comment 29•11 years ago
|
||
Attachment #727641 -
Flags: review?(jwalden+bmo)
Comment 30•11 years ago
|
||
Attachment #727642 -
Flags: review?(jwalden+bmo)
Comment 31•11 years ago
|
||
Attachment #727643 -
Flags: review?(jwalden+bmo)
Comment 32•11 years ago
|
||
Attachment #727644 -
Flags: review?(jwalden+bmo)
Comment 33•11 years ago
|
||
Attachment #727645 -
Flags: review?(jwalden+bmo)
Comment 34•11 years ago
|
||
Attachment #727646 -
Flags: review?(jwalden+bmo)
Comment 35•11 years ago
|
||
Attachment #727647 -
Flags: review?(jwalden+bmo)
Comment 36•11 years ago
|
||
Attachment #727648 -
Flags: review?(jwalden+bmo)
Comment 37•11 years ago
|
||
Attachment #727649 -
Flags: review?(jwalden+bmo)
Comment 38•11 years ago
|
||
Attachment #727650 -
Flags: review?(jwalden+bmo)
Comment 39•11 years ago
|
||
Attachment #727651 -
Flags: review?(jwalden+bmo)
Comment 40•11 years ago
|
||
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.
Comment 41•11 years ago
|
||
Attachment #727655 -
Flags: review?(jwalden+bmo)
Comment 42•11 years ago
|
||
Attachment #727657 -
Flags: review?(jwalden+bmo)
Comment 43•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(jwalden+bmo)
Comment 44•11 years ago
|
||
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 45•11 years ago
|
||
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.
Reporter | ||
Comment 46•11 years ago
|
||
(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)
Comment 47•11 years ago
|
||
Changed RequiredStringArg to using CallArgs
Attachment #727634 -
Attachment is obsolete: true
Attachment #727634 -
Flags: review?(jwalden+bmo)
Attachment #727967 -
Flags: review?(Ms2ger)
Comment 48•11 years ago
|
||
(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?
Updated•11 years ago
|
Flags: needinfo?(jwalden+bmo)
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 49•11 years ago
|
||
I'll remove IsConstructing from js/src/jsfuninlines.h Should I remove JS_IsConstructing, too?
Reporter | ||
Comment 50•11 years ago
|
||
(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)
Comment 51•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(jwalden+bmo)
Comment 52•11 years ago
|
||
Sorry, more info, I'm changing js/src/jsfun.cpp:1111
Comment 53•11 years ago
|
||
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 54•11 years ago
|
||
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+
Comment 55•11 years ago
|
||
(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.
Comment 56•11 years ago
|
||
My bad. C++'s references, which is what CallArgs::operator[] is returning, aren't supposed to be taken address of.
Flags: needinfo?(jwalden+bmo)
Comment 57•11 years ago
|
||
Attachment #728187 -
Flags: review?(jwalden+bmo)
Comment 58•11 years ago
|
||
Attachment #728188 -
Flags: review?(jwalden+bmo)
Comment 59•11 years ago
|
||
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)
Comment 60•11 years ago
|
||
Attachment #728190 -
Flags: review?(jwalden+bmo)
Comment 61•11 years ago
|
||
Attachment #728191 -
Flags: review?(jwalden+bmo)
Updated•11 years ago
|
Attachment #728010 -
Attachment is obsolete: false
Comment 62•11 years ago
|
||
Attachment #728192 -
Flags: review?(jwalden+bmo)
Comment 63•11 years ago
|
||
Sorry, I accidentally obsoleted that. It survives the merge.
Comment 64•11 years ago
|
||
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.
Reporter | ||
Comment 65•11 years ago
|
||
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)
Comment 66•11 years ago
|
||
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.
Comment 67•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(jwalden+bmo)
Comment 68•11 years ago
|
||
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)
Comment 69•11 years ago
|
||
Attachment #728187 -
Attachment is obsolete: true
Attachment #728726 -
Flags: review?(jwalden+bmo)
Comment 70•11 years ago
|
||
Attachment #728188 -
Attachment is obsolete: true
Attachment #728188 -
Flags: review?(jwalden+bmo)
Attachment #728727 -
Flags: review?(jwalden+bmo)
Comment 71•11 years ago
|
||
Attachment #728189 -
Attachment is obsolete: true
Attachment #728189 -
Flags: review?(jwalden+bmo)
Attachment #728728 -
Flags: review?(jwalden+bmo)
Comment 72•11 years ago
|
||
Attachment #728190 -
Attachment is obsolete: true
Attachment #728190 -
Flags: review?(jwalden+bmo)
Attachment #728729 -
Flags: review?(jwalden+bmo)
Comment 73•11 years ago
|
||
Attachment #728191 -
Attachment is obsolete: true
Attachment #728191 -
Flags: review?(jwalden+bmo)
Attachment #728730 -
Flags: review?(jwalden+bmo)
Comment 74•11 years ago
|
||
Attachment #728192 -
Attachment is obsolete: true
Attachment #728192 -
Flags: review?(jwalden+bmo)
Attachment #728731 -
Flags: review?(jwalden+bmo)
Comment 75•11 years ago
|
||
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
Comment 76•11 years ago
|
||
@Waldo do you have any comments for these patches?
Reporter | ||
Comment 77•11 years ago
|
||
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+
Comment 78•11 years ago
|
||
Thanks, I will do that the next time around.
Reporter | ||
Comment 79•11 years ago
|
||
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]
Comment 80•11 years ago
|
||
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.
Comment 82•11 years ago
|
||
I'm going to play my "I'm not a peer" card here... I bet evilpie could do it.
Updated•11 years ago
|
Attachment #728731 -
Flags: review?(jwalden+bmo) → review?(evilpies)
Updated•11 years ago
|
Attachment #728730 -
Flags: review?(jwalden+bmo) → review?(evilpies)
Updated•11 years ago
|
Attachment #728729 -
Flags: review?(jwalden+bmo) → review?(evilpies)
Updated•11 years ago
|
Attachment #728010 -
Flags: checkin?(Ms2ger)
Comment 83•11 years ago
|
||
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 84•11 years ago
|
||
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 85•11 years ago
|
||
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.
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Attachment #728010 -
Flags: checkin?(Ms2ger)
Updated•11 years ago
|
Keywords: checkin-needed
Comment 86•11 years ago
|
||
Comment on attachment 728010 [details] [diff] [review] New new patch for Profilers.cpp https://hg.mozilla.org/integration/mozilla-inbound/rev/f8270fefff28
Attachment #728010 -
Flags: checkin+
Reporter | ||
Comment 88•11 years ago
|
||
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+
Comment 89•11 years ago
|
||
(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?
Updated•11 years ago
|
Attachment #728726 -
Flags: checkin?(ryanvm)
Updated•11 years ago
|
Attachment #728728 -
Flags: checkin?(ryanvm)
Updated•11 years ago
|
Attachment #728726 -
Flags: checkin?(ryanvm)
Updated•11 years ago
|
Attachment #728728 -
Flags: checkin?(ryanvm)
Comment 90•11 years ago
|
||
Sorry I made a mistake asking for a checkin again.
Comment 91•11 years ago
|
||
Attachment #728729 -
Attachment is obsolete: true
Attachment #732357 -
Flags: review?(evilpies)
Comment 92•11 years ago
|
||
Attachment #728730 -
Attachment is obsolete: true
Attachment #732359 -
Flags: review?(evilpies)
Reporter | ||
Comment 93•11 years ago
|
||
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+
Reporter | ||
Comment 94•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3d7ec35485 https://hg.mozilla.org/integration/mozilla-inbound/rev/22da2932d907
Comment 95•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e3d7ec35485 https://hg.mozilla.org/mozilla-central/rev/22da2932d907
Comment 96•11 years ago
|
||
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 97•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #732359 -
Flags: review?(evilpies) → review+
Comment 98•11 years ago
|
||
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 99•11 years ago
|
||
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+
Comment 100•11 years ago
|
||
Hey Do! Are you still working on this?
Assignee | ||
Comment 101•10 years ago
|
||
Assigning myself as Do does not seem to be working on this any longer.
Assignee: mrordinaire → birunthan
Assignee | ||
Comment 102•10 years ago
|
||
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)
Assignee | ||
Comment 103•10 years ago
|
||
Attachment #8392283 -
Flags: review?(evilpies)
Assignee | ||
Comment 104•10 years ago
|
||
Attachment #8392284 -
Flags: review?(evilpies)
Assignee | ||
Comment 105•10 years ago
|
||
Attachment #8392285 -
Flags: review?(evilpies)
Assignee | ||
Comment 106•10 years ago
|
||
Attachment #8392286 -
Flags: review?(evilpies)
Assignee | ||
Comment 107•10 years ago
|
||
Attachment #8392287 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 108•10 years ago
|
||
Attachment #8392288 -
Flags: review?(vyang)
Assignee | ||
Comment 109•10 years ago
|
||
Attachment #8392289 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 110•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=33d37d7a01dc Note: This depends on bug 984112.
Attachment #8392290 -
Flags: review?(evilpies)
Comment 111•10 years ago
|
||
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 112•10 years ago
|
||
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 113•10 years ago
|
||
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 114•10 years ago
|
||
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 115•10 years ago
|
||
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 116•10 years ago
|
||
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+
Comment 117•10 years ago
|
||
(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
Reporter | ||
Comment 118•10 years ago
|
||
Also https://developer.mozilla.org/en-US/docs/SpiderMonkey/31 will need updating for this.
Reporter | ||
Comment 119•10 years ago
|
||
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 120•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8392288 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 121•10 years ago
|
||
Attachment #8392282 -
Attachment is obsolete: true
Attachment #8395006 -
Flags: review+
Assignee | ||
Comment 122•10 years ago
|
||
Attachment #8392283 -
Attachment is obsolete: true
Attachment #8395007 -
Flags: review+
Assignee | ||
Comment 123•10 years ago
|
||
Attachment #8392284 -
Attachment is obsolete: true
Attachment #8395008 -
Flags: review+
Assignee | ||
Comment 124•10 years ago
|
||
Attachment #8392285 -
Attachment is obsolete: true
Attachment #8395009 -
Flags: review+
Assignee | ||
Comment 125•10 years ago
|
||
Attachment #8392286 -
Attachment is obsolete: true
Attachment #8395011 -
Flags: review+
Assignee | ||
Comment 126•10 years ago
|
||
Attachment #8392287 -
Attachment is obsolete: true
Attachment #8395012 -
Flags: review+
Assignee | ||
Comment 127•10 years ago
|
||
Attachment #8392288 -
Attachment is obsolete: true
Attachment #8395013 -
Flags: review+
Assignee | ||
Comment 128•10 years ago
|
||
Comment 119 addressed. Bug 986076 filed for the JS OOM issue.
Attachment #8392289 -
Attachment is obsolete: true
Attachment #8395015 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 129•10 years ago
|
||
Attachment #8392290 -
Attachment is obsolete: true
Attachment #8395016 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [lang=c++][mentor=jwalden][leave open] → [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 8]
Assignee | ||
Updated•10 years ago
|
Whiteboard: [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 8] → [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 7]
Updated•10 years ago
|
Attachment #728726 -
Flags: checkin+
Updated•10 years ago
|
Attachment #728727 -
Flags: checkin+
Updated•10 years ago
|
Attachment #728728 -
Flags: checkin+
Comment 130•10 years ago
|
||
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 131•10 years ago
|
||
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 132•10 years ago
|
||
Comment on attachment 8395008 [details] [diff] [review] Part 3: Use JS::CallArgs instead of `argc` in js/src/ https://hg.mozilla.org/integration/mozilla-inbound/rev/e425b681d19f
Attachment #8395008 -
Flags: checkin+
Comment 133•10 years ago
|
||
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 134•10 years ago
|
||
Comment on attachment 8395011 [details] [diff] [review] Part 5: Use JS::CallArgs instead of `vp` in js/src/ https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebc8c3099ef
Attachment #8395011 -
Flags: checkin+
Comment 135•10 years ago
|
||
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 136•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [lang=c++][mentor=jwalden][leave open][checkin parts 1 - 7] → [lang=c++][mentor=jwalden][leave open]
Comment 137•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/92d2569932a4 https://hg.mozilla.org/mozilla-central/rev/e2e8f128c7a8 https://hg.mozilla.org/mozilla-central/rev/e425b681d19f https://hg.mozilla.org/mozilla-central/rev/595c8c60d676 https://hg.mozilla.org/mozilla-central/rev/0ebc8c3099ef https://hg.mozilla.org/mozilla-central/rev/58e088726ab7 https://hg.mozilla.org/mozilla-central/rev/a43111585107
Reporter | ||
Comment 138•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 139•10 years ago
|
||
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+
Comment 140•10 years ago
|
||
Comment on attachment 8395016 [details] [diff] [review] Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL} https://hg.mozilla.org/integration/mozilla-inbound/rev/726e81b3c1a9
Attachment #8395016 -
Flags: checkin+
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [lang=c++][mentor=jwalden][leave open] → [lang=c++][mentor=jwalden]
https://hg.mozilla.org/mozilla-central/rev/e53e37d2c2bf https://hg.mozilla.org/mozilla-central/rev/726e81b3c1a9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•4 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•