Bug 169832 - [JSC] Remove unnecessary condition from needsDerivedConstructorInArrowFunctionLexicalEnvironment in BytecodeGenerator.cpp
Summary: [JSC] Remove unnecessary condition from needsDerivedConstructorInArrowFunctio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 169647
Blocks:
  Show dependency tree / graph
 
Reported: 2017-03-17 13:40 PDT by GSkachkov
Modified: 2017-03-20 10:09 PDT (History)
7 users (show)

See Also:

Attachments
Patch (2.04 KB, patch)
2017-03-17 14:15 PDT, GSkachkov 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (868.98 KB, application/zip)
2017-03-17 15:27 PDT, Build Bot 		no flags Details
Patch (2.04 KB, patch)
2017-03-18 01:47 PDT, GSkachkov 		mark.lam: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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