Bug 164996 – Fix exception scope verification failures in *Executable.cpp files.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 295275 [details]
proposed patch.

[Darin Adler]

Comment on attachment 295275 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ProgramExecutable.cpp:82
>      JSObject* exception = 0;

nullptr

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:314
> +        ExecState* exec = scope->globalObject()->globalExec();

I would write:

    auto& state = *scope->globalObject()->globalExec();

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:318
>      JSObject* exception = 0;

nullptr

[Saam Barati]

Comment on attachment 295275 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:314
>> +        ExecState* exec = scope->globalObject()->globalExec();
> 
> I would write:
> 
>     auto& state = *scope->globalObject()->globalExec();

I think making this change would go against the style of what we usually do inside JSC. I'm not against using this style throughout JSC, however, I think it's more important that we be consistent. I think at some point it's worth all of us having a more detailed discussion about coding style in JSC. I know that we mostly follow the normal WebKit style, however, I've noticed that we've differed from that style in some ways. Notably, I think JSC uses "auto" much less than the rest of WebKit does.

In this specific line of code, I usually see it written as Mark has written it. We tend to call things of type "ExecState*" exec, and not declare it using auto.

[Mark Lam]

Comment on attachment 295275 [details]
proposed patch.

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

>>> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:314
>>> +        ExecState* exec = scope->globalObject()->globalExec();
>> 
>> I would write:
>> 
>>     auto& state = *scope->globalObject()->globalExec();
> 
> I think making this change would go against the style of what we usually do inside JSC. I'm not against using this style throughout JSC, however, I think it's more important that we be consistent. I think at some point it's worth all of us having a more detailed discussion about coding style in JSC. I know that we mostly follow the normal WebKit style, however, I've noticed that we've differed from that style in some ways. Notably, I think JSC uses "auto" much less than the rest of WebKit does.
> 
> In this specific line of code, I usually see it written as Mark has written it. We tend to call things of type "ExecState*" exec, and not declare it using auto.

I think I'll go ahead and cave, and switch to using the convention of using ExecState& state instead where possible since I'll be touching a lot of code in my exception checks series of patches.  I don't see the WebKit project switching back to using "exec".  So, might as well get started on the switch.

[Mark Lam]

Thanks for the reviews.  Landed in r208950: <http://trac.webkit.org/r208950>.