From: Paul Gofman <pgofman@codeweavers.com>
Subject: Re: [PATCH v4 2/6] jscript: Enumerate with and block scopes.
Message-Id: <583342c2-5b8b-850a-84fa-21cb793c6f5e@codeweavers.com>
Date: Fri, 18 Jun 2021 14:40:21 +0300
In-Reply-To: <fac2ed2f-a7ef-1d50-77dd-b9328efd365f@codeweavers.com>
References: <20210617212523.1313394-1-pgofman@codeweavers.com> <20210617212523.1313394-2-pgofman@codeweavers.com> <fac2ed2f-a7ef-1d50-77dd-b9328efd365f@codeweavers.com>

Hello Jacek,

On 6/18/21 13:16, Jacek Caban wrote:
> Hi Paul,
>
>
> Patches look mostly good to me now, I have one doubt through:
>
> On 6/17/21 11:25 PM, Paul Gofman wrote:
>>       case STAT_WITH: {
>>           with_statement_t*with_stat = (with_statement_t*)stat;
>> +        statement_ctx_t stat_ctx = {0, TRUE};
>>             hres = visit_expression(ctx, with_stat->expr);
>>           if(FAILED(hres))
>>               return hres;
>>   -        hres = visit_statement(ctx, with_stat->statement);
>> +        if (ctx->parser->script->version >= SCRIPTLANGUAGEVERSION_ES5)
>> +        {
>> +            if (!alloc_local_scope(ctx, &with_stat->scope_index))
>> +                return E_OUTOFMEMORY;
>> +
>> +            stat_ctx.scope_index = with_stat->scope_index;
>> +        }
>
>
> Do we need to alloc local scope for with statement? They only use
> passed object for the scope and, unless I'm missing something, we
> should never need this local scope anyway.

I recall it was needed for something at some earlier version of the
patches, but I checked now, and it actually seems redundant. More so, I
tested the function definition under 'with' without explicit block, and
it revealed some bugs WRT current 'with' scope enumeration handling, as
well that the 'with' doesn't affect the function definition ('with'
scope shouldn't be attached to the function).

So, I will add the test and remove the scope enumeration for 'with' and
dependent parts.


I also tried testing:

    with({w:10})
        let e = w

But figured that is syntax error. LexicalDeclaration shouldn't be
actually a part of Statement but rather a part of a new Declaration
statement which should be referenced from Block statement declaration. I
suggest I fix that after this patch series.