1383775 - Clean up the function toString/toSource code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Forgot to update BindingUtils.cpp

Comment 2

[André Bargull [:anba]]

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?

Comment 3

[André Bargull [:anba]]

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

Comment 4

[Jan de Mooij [:jandem]]

(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 :)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e0e609aa3ca
Clean up function toString/toSource code, remove remnants of source decompiler. r=anba

Comment 6

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> I'll also update these docs :)

\o/

Comment 7

[Jan de Mooij [:jandem]]

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...)

Comment 8

[j.j.]

(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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e0e609aa3ca