Closed
Bug 1383775
Opened 7 years ago
Closed 7 years ago
Clean up the function toString/toSource code
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file, 1 obsolete file)
31.16 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Every time I look at this code, I get super confused by the indent/prettyPrint/JS_DONT_PRETTY_PRINT cruft. This patch removes JS_DONT_PRETTY_PRINT and replaces the indent/prettyPrint arguments with a |bool isToSource| to make it clear this is the weird/non-standard toSource extension.
Attachment #8889481 -
Flags: review?(andrebargull)
Assignee | ||
Comment 1•7 years ago
|
||
Forgot to update BindingUtils.cpp
Attachment #8889481 -
Attachment is obsolete: true
Attachment #8889481 -
Flags: review?(andrebargull)
Attachment #8889502 -
Flags: review?(andrebargull)
Comment 2•7 years ago
|
||
Comment on attachment 8889502 [details] [diff] [review] Patch Review of attachment 8889502 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! Do we have any policy about JSAPI signature changes? Just curious because in bug 761723 we kept the |name| parameter for JS_DecompileScript, even though it's no longer used. And can you also rename AsmJSModuleToString's |addParenToLambda| parameter to |isToSource| [1]? Thanks! [1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/wasm/AsmJS.h#83 ::: js/src/jsapi.h @@ +4358,5 @@ > > } /* namespace JS */ > > extern JS_PUBLIC_API(JSString*) > +JS_DecompileScript(JSContext* cx, JS::Handle<JSScript*> script, const char* name); Given that we're already changing the signature of JS_DecompileScript by removing |indent|, do you think it makes sense to also remove |name|. (It's unused since https://hg.mozilla.org/mozilla-central/diff/e080642175e6/js/src/jsapi.cpp, bug 761723 from five years ago.) ::: js/src/jsfun.cpp @@ -1148,5 @@ > > - uint32_t indent = 0; > - > - if (args.length() != 0 && !ToUint32(cx, args[0], &indent)) > - return false; Good to see this go away! ::: js/src/jsopcode.cpp @@ -1208,5 @@ > JSObject& obj = v.toObject(); > > if (obj.is<JSFunction>()) { > RootedFunction fun(cx, &obj.as<JSFunction>()); > - JSString* str = JS_DecompileFunction(cx, fun, JS_DONT_PRETTY_PRINT); I guess nobody really minds if ToDisassemblySource no longer prints parentheses around lambda functions?
Attachment #8889502 -
Flags: review?(andrebargull) → review+
Comment 3•7 years ago
|
||
And we may want to update https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_DecompileFunction and https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_DecompileScript. (Not that we really care about the MDN pages for JSAPI.... :-P
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to André Bargull from comment #2) > Do we have any policy about JSAPI signature changes? Just curious because in > bug 761723 we kept the |name| parameter for JS_DecompileScript, even though > it's no longer used. We don't have a policy for this I think - we change JSAPI signatures all the time. I'll remove the name argument, good catch. > And can you also rename AsmJSModuleToString's |addParenToLambda| parameter > to |isToSource| [1]? Thanks! Sure. > I guess nobody really minds if ToDisassemblySource no longer prints > parentheses around lambda functions? Yeah I think this is just for dumping bytecode and having some sort of representation for nested functions etc. I don't think the exact format matters too much. I'll also update these docs :)
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e0e609aa3ca Clean up function toString/toSource code, remove remnants of source decompiler. r=anba
Comment 6•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #4) > I'll also update these docs :) \o/
Assignee | ||
Comment 7•7 years ago
|
||
I updated the signatures and removed the arguments from the documentation, but note that the description in these docs is still pretty outdated. (And we should probably rename JS_Decompile{Function,Script} to JS::{Function,Script}ToString at some point...)
(In reply to Jan de Mooij [:jandem] from comment #7) > I updated the signatures and removed the arguments from the documentation, > but note that the description in these docs is still pretty outdated. BTW, there are "dev-doc-needed" and "dev-doc-complete" keywords for those issues.
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e0e609aa3ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•