[Webkit-unassigned] [Bug 163208] [ES6]. Implement Annex B.3.3 function hoisting rules for eval

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 13 19:33:08 PDT 2017


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

--- Comment #69 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 306399
  --> https://bugs.webkit.org/attachment.cgi?id=306399
Patch

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

Almost everything LGTM, just 1 fundamental question below:

> Source/JavaScriptCore/bytecode/UnlinkedEvalCodeBlock.h:60
> +    void adoptFunctionHoistingCandidates(Vector<Identifier, 0, UnsafeVectorOverflow>& functionHoistingCandidates)
> +    {
> +        ASSERT(m_functionHoistingCandidates.isEmpty());
> +        m_functionHoistingCandidates.swap(functionHoistingCandidates);
> +    }

Style nit: You can use C++ move semantics here and do something like:
adopt(Vector<T>&& v) { ASSERT(m_v.isEmpty()); m_v = WTFMove(v); }

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:771
> +        if (!entry.value.isSloppyModeHoistingCandidate())
> +            variables.append(Identifier::fromUid(m_vm, entry.key.get()));
> +        else
> +            hoistedFunctions.append(Identifier::fromUid(m_vm, entry.key.get()));

Style nit:
I think this is easier to read if you flip the if and else and remove the "!" on the if's condition.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2225
> +            ASSERT_UNUSED(checkResult.get(), checkResult.get() != nullptr);

no need for ASSERT_UNUSED here, can just be a regular ASSERT.
ASSERT_UNUSED is only for variables, not temp expressions

> Source/JavaScriptCore/parser/Parser.h:1201
> +            bool isSloppyModeHoistingCandidate = !strictMode() && closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext();

This can only be true if m_statementDepth == 1, however, isn't this just a top level function if m_statementDepth == 1? Why is this needed here then?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170414/562d0cdf/attachment.html>


More information about the webkit-unassigned mailing list