[PATCH 1/4] vbscript: Add support for the SCRIPTTEXT_ISEXPRESSION flag in ParseScriptText.

[Jacek Caban]

Hi Gabriel,

The patch looks mostly good, a few comments below.

On 9/16/19 3:05 PM, Gabriel Ivăncescu wrote:
> To simplify the amount of special cases both in ParseScriptText and
> ParseProcedureText, a new pseudo statement and opcode have been added to
> return the expression and value at the top of the stack, respectively. Script
> texts that have this flag will be parsed specially as a single expression
> with such a statement at the end.
>
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> ---
>   dlls/vbscript/compile.c  | 22 ++++++++++++++++++++--
>   dlls/vbscript/interp.c   | 29 +++++++++++++++++++++++++++--
>   dlls/vbscript/lex.c      |  7 +++++++
>   dlls/vbscript/parse.h    | 11 +++++++++--
>   dlls/vbscript/parser.y   | 37 ++++++++++++++++++++++++++++++++++---
>   dlls/vbscript/vbscript.c | 14 +++++++-------
>   dlls/vbscript/vbscript.h |  3 ++-
>   7 files changed, 106 insertions(+), 17 deletions(-)
>
> diff --git a/dlls/vbscript/compile.c b/dlls/vbscript/compile.c
> index 1f4a7d3..0527297 100644
> --- a/dlls/vbscript/compile.c
> +++ b/dlls/vbscript/compile.c
> @@ -1196,6 +1196,21 @@ static HRESULT compile_onerror_statement(compile_ctx_t *ctx, onerror_statement_t
>       return push_instr_int(ctx, OP_errmode, stat->resume_next);
>   }
>   
> +static HRESULT compile_retval_statement(compile_ctx_t *ctx, retval_statement_t *stat)
> +{
> +    HRESULT hres;
> +
> +    hres = compile_expression(ctx, stat->expr);
> +    if(FAILED(hres))
> +        return hres;
> +
> +    hres = push_instr(ctx, OP_retval);
> +    if(FAILED(hres))
> +        return hres;
> +
> +    return S_OK;
> +}
> +
>   static HRESULT compile_statement(compile_ctx_t *ctx, statement_ctx_t *stat_ctx, statement_t *stat)
>   {
>       HRESULT hres;
> @@ -1267,6 +1282,9 @@ static HRESULT compile_statement(compile_ctx_t *ctx, statement_ctx_t *stat_ctx,
>           case STAT_WHILELOOP:
>               hres = compile_while_statement(ctx, (while_statement_t*)stat);
>               break;
> +        case STAT_RETVAL:
> +            hres = compile_retval_statement(ctx, (retval_statement_t*)stat);
> +            break;


As a side note, for a possible future optimization, we could also try to 
statically catch assignments to retval and emit OP_retval for them as well.


> +static HRESULT interp_retval(exec_ctx_t *ctx)
> +{
> +    variant_val_t val;
> +    HRESULT hres;
> +
> +    TRACE("\n");
> +
> +    hres = stack_pop_val(ctx, &val);
> +    if(FAILED(hres))
> +        return hres;
> +
> +    if(val.owned)
> +        ctx->ret_val = *val.v;
> +    else {
> +        VARIANT v;
> +
> +        V_VT(&v) = VT_EMPTY;
> +        hres = VariantCopy(&v, val.v);
> +        if(FAILED(hres))
> +            return hres;
> +
> +        ctx->ret_val = v;


Technically you're leaking previous ret_val value (although current 
compiler would never emit leaking code). You could just pass ret_val as 
VariantCopy destination.


> diff --git a/dlls/vbscript/lex.c b/dlls/vbscript/lex.c
> index 4bcf810..d527c35 100644
> --- a/dlls/vbscript/lex.c
> +++ b/dlls/vbscript/lex.c
> @@ -493,6 +493,13 @@ int parser_lex(void *lval, parser_ctx_t *ctx)
>   {
>       int ret;
>   
> +    if (ctx->start_token)
> +    {
> +        int tmp = ctx->start_token;
> +        ctx->start_token = 0;
> +        return tmp;
> +    }


Could we use already existing last_token here?

if(ctx->last_token == tEXPRESSION) {

     ctx->last_token = tNL;

     return tEXPRESSION;

}


> @@ -81,7 +82,9 @@ static statement_t *link_statements(statement_t*,statement_t*);
>   %lex-param { parser_ctx_t *ctx }
>   %parse-param { parser_ctx_t *ctx }
>   %pure-parser
> -%start Program
> +%start Start
> +%token START_PROGRAM


Do you really need both START_PROGRAM and START_EXPRESSION? It seems to 
me that having just one of them should be enough.


> +%token START_EXPRESSION


tEXPRESSION or tSTART_EXPRESSION would be more consistent with other tokens.


Thanks,

Jacek