Closed
Bug 719294
Opened 13 years ago
Closed 13 years ago
cx-less js::PCToLineNumber
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(1 file, 2 obsolete files)
11.84 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Implement a js_MapPCToLineNumber that does not require a JSContext, but only needs the data from a script needed to perform the mapping
Assignee | ||
Comment 1•13 years ago
|
||
Eliminate the bytecode space optimization where it omits the starting line number as per this comment:
/*
* Special case: function definition needs no line number note because
* the function's script contains its starting line number.
*/
But this is the only reason why you need a JSContext in js_PCToLineNumber, and I have a user that needs to do the lookup without a context.
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #589715 -
Flags: review?(luke)
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 589730 [details] [diff] [review]
Implement js_MapPCToLineNumber for when you don't have a JSScript, just the necessary fragments for doing the mapping
This operation is needed for doing JS backtraces that involve scripts that have been deleted since the backtrace was taken.
Attachment #589730 -
Flags: review?(luke)
![]() |
||
Comment 4•13 years ago
|
||
Comment on attachment 589715 [details] [diff] [review]
Eliminate the bytecode space optimization where it omits the starting line number
> JSOp op = fun->isFlatClosure() ? JSOP_DEFFUN_FC : JSOP_DEFFUN;
>+
> EMIT_INDEX_OP(op, index);
No need to add the space: with C++, since the decls don't need to be hoisted, we don't use the \n to separate decls from uses.
>diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp
>+#ifdef DEBUG
>+ if (cx) {
This DEBUG block seems like a great thing to run through try server but, once you've done that, I think it would be nicer to remove it so that you can remove the 'cx' parameter altogether
>+ * walk through source notes accumulating their deltas, keeping track of
Capitalize Walk
Attachment #589715 -
Flags: review?(luke) → review+
![]() |
||
Comment 5•13 years ago
|
||
Comment on attachment 589730 [details] [diff] [review]
Implement js_MapPCToLineNumber for when you don't have a JSScript, just the necessary fragments for doing the mapping
Nice. Instead of having the two names, begging the question "what is Map vs. non-Map", how about giving both functions the same name: the signatures speak for themselves as will a quick look at the definition of the JSScript-taking overload. Lastly, in the interest of incremental-modernization, could you rename both to js::PCToLineNumber?
Attachment #589730 -
Flags: review?(luke) → review+
Assignee | ||
Comment 6•13 years ago
|
||
You suggested eliminating the cx parameter. Lemme tag you for re-review to be sure you meant it. (Public API does not change.)
Attachment #594393 -
Flags: review?(luke)
Assignee | ||
Updated•13 years ago
|
Attachment #589715 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #589730 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Version: 12 Branch → Trunk
Assignee | ||
Updated•13 years ago
|
Assignee: general → sphink
Summary: js_MapPCToLineNumber → cx-less js::PCToLineNumber
![]() |
||
Comment 7•13 years ago
|
||
Comment on attachment 594393 [details] [diff] [review]
Eliminate the bytecode space optimization where it omits the starting line number
Mmm sweet
Attachment #594393 -
Flags: review?(luke) → review+
Assignee | ||
Comment 8•13 years ago
|
||
Target Milestone: --- → mozilla12
Comment 9•13 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3328813a98 - something in that push burned.
Target Milestone: mozilla12 → ---
Assignee | ||
Comment 10•13 years ago
|
||
Yes, it was this one that burned it. Turned out to be a bad merge. I ended up pushing code that looked like
if (!operation1())
if (!operation2())
return false;
return false;
which didn't do quite what I wanted. I'm not crazy about significant whitespace, but I wouldn't mind warnings based on whitespace...
Re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90f6626365e3
Target Milestone: --- → mozilla13
Comment 11•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•