Change all functions taking CallArgs to take const CallArgs& instead

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: till, Assigned: till)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

Turns out there are slightly more non-generic functions than I, naively, thought. I fixed all of them to take `const CallArgs&`. Also, since I was at it, I changed all other occurrences of CallArgs in the same way.
Test failures are caused by faulty base revision.
Comment on attachment 8652256 [details] [diff] [review]
Change all functions taking CallArgs to take const CallArgs& instead

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

Land it land it land it quick!

::: js/src/jsapi.cpp
@@ +117,5 @@
>  #define JS_ADDRESSOF_VA_LIST(ap) (&(ap))
>  #endif
>  
>  bool
> +JS::CallArgs::requireAtLeast(JSContext* cx, const char* fnname, unsigned required) const {

Bump the { onto its own line while you're here.
Attachment #8652256 - Flags: review?(jwalden+bmo) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c59612f82a95c89661dbbaf28e04854b7e1c7e19
changeset:  c59612f82a95c89661dbbaf28e04854b7e1c7e19
user:       Till Schneidereit <till@tillschneidereit.net>
date:       Thu Aug 27 21:18:37 2015 +0200
description:
Bug 1198193 - Change all functions taking CallArgs to take const CallArgs& instead. r=Waldo
https://hg.mozilla.org/mozilla-central/rev/c59612f82a95
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
I hope you don't mind me asking a general question about this change here, since I seem to be lacking some context/background:
Why, exactly, is this desired? What's the advantage of this change?
See the 2-minutes discussion starting here:
http://logs.glob.uno/?c=mozilla%23jsapi&s=12+Mar+2015&e=12+Mar+2015&h=CallArgs#c560488

(And yes, it would sure have been nice to have that in this bug or even in the patch.)
Thanks - much appreciated.

So, for the record and future reference, this was changed because passing CallArgs as an object to functions instead of using a reference doesn't pass the usedRval debugging flag in that object back to the caller. That in turn would make calleev() never assert, even when it -should-, because there's no record of rval() having been called in the function.

Did I understand that correctly? i.e.: essential for debugging, but not for run-time execution?
Yes, that's correct. Additionally it makes stack frames for these functions a tiny bit smaller, which isn't bad, but it also doesn't matter much because CallArgs really isn't big.

Out of curiosity, why're you interested in this patch in particular? It doesn't seem that notable a change.
(In reply to Till Schneidereit [:till] from comment #10)
> Out of curiosity, why're you interested in this patch in particular? It
> doesn't seem that notable a change.

I ran across it when looking at porting other JS engine code to what I'm working on (since it's touching a lot of files, the reference popped up a few times) and it wasn't clear why it was needed at first glance, but still had comment 4 display urgency to land it. So of course it piqued my interest as a result ;)

It's not as notable and considered low priority now I know what it's all about; it'd still be a good thing to take since it's obviously fixing something that's broken otherwise (and smaller stack frames is always good).
> I ran across it when looking at porting other JS engine code to what I'm
> working on (since it's touching a lot of files, the reference popped up a
> few times) and it wasn't clear why it was needed at first glance, but still
> had comment 4 display urgency to land it. So of course it piqued my interest
> as a result ;)

Ah, that makes sense. I think Jeff was mostly concerned about bitrot: as you said, this touches a lot of files, so it's likely to be affected by other patches. Also, I think he just really liked the change :)
You need to log in before you can comment on or make changes to this bug.