Re: [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
>
> 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