Closed Bug 1383775 Opened 7 years ago Closed 7 years ago

Clean up the function toString/toSource code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — 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)
Attached patch PatchSplinter Review
Forgot to update BindingUtils.cpp
Attachment #8889481 - Attachment is obsolete: true
Attachment #8889481 - Flags: review?(andrebargull)
Attachment #8889502 - Flags: review?(andrebargull)
Blocks: 1383343
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+
(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
(In reply to Jan de Mooij [:jandem] from comment #4)
> I'll also update these docs :)

\o/
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.
https://hg.mozilla.org/mozilla-central/rev/2e0e609aa3ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.