[PATCH v3 3/6] jscript: Support block scope variables.

[Paul Gofman]

On 6/16/21 22:35, Jacek Caban wrote:
>
>
> It would be nice to split it a bit. For example, it seems to me that
> visiting phrase changes could be a separated patch.
Well, I even had most of the visiting part as a separate patch initially
but squashed all that after some consideration. I will pull it back.
>
>
>>
>>   +#define MAX_SCOPE_COUNT 1024
>
>
> Such arbitrary limit seems bad. Scripts can be really long, so it's
> not unrealistic to hit the limit. At the same time, it's wasteful for
> short scripts like "runOnload()".
>
I will switch that to dynamic reallocation.


>
>>
>> +            TRACE("iter->scope_index %d,
>> ctx->func->local_scope_count %d.\n", iter->scope_index,
>> ctx->func->local_scope_count);
>
>
> Is it a debugging leftover? I wouldn't mind adding some debug traces,
> but this one seems kind of random.
>
It is not exactly a leftover but yes, it is not particularly useful. I
will remove it.


>   {
>> +    statement_ctx_t stat_ctx = {0, TRUE};
>> +    BOOL needs_scope;
>>       HRESULT hres;
>>   +    needs_scope = block && block->scope_index;
>> +    if (needs_scope)
>> +    {
>> +        if(!push_instr(ctx, OP_new_obj))
>> +            return E_OUTOFMEMORY;
>> +        if(FAILED(hres = push_instr_uint_uint(ctx, OP_push_scope,
>> block->scope_index, TRUE)))
>> +            return hres;
>
>
> Creating a new object on each block entry is a rather expensive. Could
> we create those object on demand instead (like in
> detach_variable_object())? Given that, it seems to me that a new
> dedicated opcode would appropriate here.
I should note that it is not for each block but only for blocks which
has any 'let' variables or functions defined in it. But it won't hurt to
optimize for the case when there are just local variables and the scope
ends up not being referenced on leave. I will add the opcode and create
the object on detach.