Bug 152552 - JSC option --xxx should imply --xxx=true for boolean options.
Summary: JSC option --xxx should imply --xxx=true for boolean options.
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-24 17:51 PST by Mark Lam
Modified: 2015-12-31 15:11 PST (History)
5 users (show)

See Also:


Attachments
proposed patch. (2.26 KB, patch)
2015-12-24 17:57 PST, Mark Lam
dbates: review+
dbates: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-12-24 17:51:13 PST
For example, we should be able to run jsc as follows:
$ jsc --reportCompileTimes myscript.js

... instead of having to say:
$ jsc --reportCompileTimes=true myscript.js

The "=true" part seems redundant.  This is not a big deal, but it's been irking me for a few years now.  Since it's easily remedied, I thought I'd roll a patch for it.
Comment 1 Mark Lam 2015-12-24 17:57:03 PST
Created attachment 267907 [details]
proposed patch.
Comment 2 Daniel Bates 2015-12-31 15:11:06 PST
Comment on attachment 267907 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=267907&action=review

> Source/JavaScriptCore/jsc.cpp:1890
> +    const char* equalStr = strchr(arg, '=');

This variable is used exactly once and I do not feel that its name improves the readability of this code. I suggest that we inline the value of this variable into the if condition expression on line 1892.

> Source/JavaScriptCore/jsc.cpp:1893
> +        impliedOption = String(arg) + "=true";

This will malloc() twice: once for a buffer of size strlen(arg) and once for a buffer whose size is strlen(arg) + strlen("=true"). Making use of the convenience function makeString() and ASCIILiteral() I would write this line as:

impliedOption = makeString(arg, ASCIILiteral("=true"));

The benefit of this approach is that it will malloc() once for a buffer the size of the result of the string concatenation.

Note, I am assuming JSC::Options::setOption() makes a copy of the string if it stores it.

On another note, it's unfortunate that we do not have a more direct way to set a boolean option in JSC other than concatentating "=true". The proposed approach is sufficient.