[Webkit-unassigned] [Bug 90420] JSC: Add ability to symbolically set and dump JSC VM options

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90420





--- Comment #14 from Filip Pizlo <fpizlo at apple.com>  2012-07-02 21:09:19 PST ---
(From update of attachment 150523)
View in context: https://bugs.webkit.org/attachment.cgi?id=150523&action=review

I'm happy with this patch!  Please consider the suggestions though, and mark as r? cq? when you're ready.

> Source/JavaScriptCore/jsc.cpp:618
> +    fprintf(stderr, "  --options                  Dumps all JSC VM options and exits\n");

Typo: "and exit"

> Source/JavaScriptCore/jsc.cpp:619
> +    fprintf(stderr, "  --dumpOptions              Dumps all JSC VM options before continuing\n");

Is this option needed?

> Source/JavaScriptCore/runtime/Options.h:34
> +// How JSC VM options work?

Typo: "How do JSC VM options work?"

> Source/JavaScriptCore/runtime/Options.h:153
> +        TYPE_bool,
> +        TYPE_unsigned,
> +        TYPE_double,
> +        TYPE_int32_t

I realize that there's no good way of making these follow naming conventions due to the need to macro-concatenate things, but it would be better to at least follow the rule that all-caps is reserved for the names of macros.  As such, I think it would be better to use "Type_bool" than "TYPE_bool".

>> Source/JavaScriptCore/runtime/Options.h:162
>> +            bool boolVal;
>> +            unsigned unsignedVal;
>> +            double doubleVal;
>> +            int32_t int32_tVal;
> 
> int32_tVal is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

One way to make this nicer would be to make these Val_bool rather than boolVal.  Then, the naming here would match the naming of types.  Would that work with the macro magic?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.