Bug 169832

Summary: [JSC] Remove unnecessary condition from needsDerivedConstructorInArrowFunctionLexicalEnvironment in BytecodeGenerator.cpp
Product: WebKit Reporter: GSkachkov <gskachkov>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, keith_miller, mark.lam, msaboff, rniwa, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 169647    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Patch mark.lam: review+, mark.lam: commit-queue-

Description GSkachkov 2017-03-17 13:40:03 PDT
Remove unnecessary condition from needsDerivedConstructorInArrowFunctionLexicalEnvironment in BytecodeGenerator.cpp
Comment 1 GSkachkov 2017-03-17 14:15:50 PDT
Created attachment 304823 [details]
Patch

Patch
Comment 2 Build Bot 2017-03-17 15:27:06 PDT
Comment on attachment 304823 [details]
Patch

Attachment 304823 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3352057

New failing tests:
fast/mediastream/apply-constraints-audio.html
Comment 3 Build Bot 2017-03-17 15:27:10 PDT
Created attachment 304828 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 4 GSkachkov 2017-03-18 01:47:46 PDT
Created attachment 304865 [details]
Patch

Updated Patch
Comment 5 Mark Lam 2017-03-18 09:08:21 PDT
Comment on attachment 304865 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        isConstructor.

I think you mean "isClassContext" here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1031
> -    if ((isConstructor() && constructorKind() == ConstructorKind::Extends) || m_codeBlock->isClassContext()) {
> -        if (isSuperUsedInInnerArrowFunction())
> -            return true;
> -    }
> -    return false;
> +    return m_codeBlock->isClassContext() && isSuperUsedInInnerArrowFunction();

I understand that m_codeBlock->isClassContext() means that "executable->superBinding() == SuperBinding::Needed", and that in turn is true because we set "expectedSuperBinding = m_defaultConstructorKind == ConstructorKind::Extends ? SuperBinding::Needed : SuperBinding::NotNeeded".  However, this relationship is a few steps removed.  Can you add an assert before this so that the relationship doesn't change without us noticing?
    ASSERT(m_codeBlock->isClassContext() || !(isConstructor() && constructorKind() == ConstructorKind::Extends));
Comment 6 GSkachkov 2017-03-18 13:58:28 PDT
Committed r214138: <http://trac.webkit.org/changeset/214138>

All reviewed patches have been landed.  Closing bug.
Comment 7 GSkachkov 2017-03-20 10:09:03 PDT
Fixed and landed