Re: [PATCH] pkl,doc,testsuite: Add `assert` statement

[Jose E. Marchesi]

Hi Mohammad!
Thanks for the new version of the patch.

>  /* Standard exception codes.
> +   These codes should be in sync with PVM_E_* macros in pvm.h.
>     Note that user-defined exceptions must have codes starting with
>     255.
>     Note also that EC_generic _must_ be zero.  */

Thanks for adding that comment.  I should have done it myself long ago,
to complement the one in pvm.h :)

> +/* Assertion function.
> +
> +   Poke parser transforms assert statement to invocation of this
> +   function.

No need to refer to the parser in particular.  We may change the
implementation of the compiler at some point and move the transformation
to one of the `trans' phases.

Better say "The compiler transforms...".

> diff --git a/libpoke/pkl-tab.y b/libpoke/pkl-tab.y
> index ba3ff6e9..7b1b138c 100644
> --- a/libpoke/pkl-tab.y
> +++ b/libpoke/pkl-tab.y
> @@ -120,6 +120,81 @@ pkl_register_arg (struct pkl_parser *parser, 
> pkl_ast_node arg)
>    return 1;
>  }
>  
> +/* Assert statement is a syntatic sugar that transforms to invocation of
> +   _pkl_assert function with appropriate arguments.
> +
> +   This function accepts AST nodes corresponding to the condition and
> +   optional message of the assert statement, and also the location info
> +   of the statement.
> +
> +   Returns NULL on failure, and expression statement AST node on success.  */
> +
> +static pkl_ast_node
> +pkl_make_assertion (struct pkl_parser *p, pkl_ast_node cond, pkl_ast_node 
> msg,
> +                    struct pkl_ast_loc stmt_loc)
> +{
> +  pkl_ast_node vfunc, call, asrt;
> +  pkl_ast_node arg_cond, arg_msg, arg_lineinfo; /* _pkl_assert args */
> +  const char *name = "_pkl_assert"; /* Function defined in pkl-rt.pk */

Wouldn't the variable `name' belong to the compound statement below?

> +
> +  /* Make variable for `_pkl_assert` */
> +  {
> +    pkl_ast_node vfunc_init;
> +    int back, over;
> +
> +    vfunc_init = pkl_env_lookup (p->env, PKL_ENV_NS_MAIN, name, &back, 
> &over);
> +    if (!vfunc_init
> +        || (PKL_AST_DECL_KIND (vfunc_init) != PKL_AST_DECL_KIND_FUNC))
> +      {
> +        pkl_error (p->compiler, p->ast, stmt_loc, "undefined function '%s'",
> +                   name);
> +        return NULL;
> +      }
> +    vfunc = pkl_ast_make_var (p->ast, pkl_ast_make_identifier (p->ast, name),
> +                              vfunc_init, back, over);
> +  }
> +
> +  /* First argument of _pkl_assert */
> +  arg_cond = pkl_ast_make_funcall_arg (p->ast, cond, NULL);
> +  PKL_AST_LOC (arg_cond) = PKL_AST_LOC (cond);

I don't think this location is necessary.

> +
> +  /* Second argument of _pkl_assert */
> +  if (msg == NULL)
> +    {
> +      msg = pkl_ast_make_string (p->ast, "");
> +      PKL_AST_TYPE (msg) = ASTREF (pkl_ast_make_string_type (p->ast));
> +    }
> +  arg_msg = ASTREF (pkl_ast_make_funcall_arg (p->ast, msg, NULL));
> +  PKL_AST_LOC (arg_msg) = PKL_AST_LOC (msg);

Likewise.

> +
> +  /* Third argument of _pkl_assert to report the location of the assert
> +     statement with the following format "<FILENAME>:<LINE>:<COLUMN>".  */
> +  {
> +    char *str;
> +    pkl_ast_node lineinfo;
> +
> +    if (asprintf (&str, "%s:%d:%d", p->filename ? p->filename : "<stdin>",
> +                  stmt_loc.first_line, stmt_loc.first_column)
> +        == -1)
> +      return NULL;
> +    lineinfo = pkl_ast_make_string (p->ast, str);
> +    free (str);
> +    PKL_AST_TYPE (lineinfo) = ASTREF (pkl_ast_make_string_type (p->ast));

That won't work.  ASTREF operates on l-values.  You will have to do:

pkl_ast_node string_type = pkl_ast_make_string_type (p->ast);
PKL_AST_TYPE (lineinfo) = ASTREF (string_type);

This is documented in HACKING ;)

> +
> +    arg_lineinfo = ASTREF (pkl_ast_make_funcall_arg (p->ast, lineinfo, 
> NULL));

Likewise.

> +  }
> +
> +  call = pkl_ast_make_funcall (
> +      p->ast, vfunc,
> +      pkl_ast_chainon (arg_cond, pkl_ast_chainon (arg_msg, arg_lineinfo)));

Please do not put a newline between a ( and the first argument.  Break
before the = instead, in case you need to.

> +  PKL_AST_LOC (call) = stmt_loc;

I don't think setting this location is necessary.

> +
> +  asrt = pkl_ast_make_exp_stmt (p->ast, call);
> +  PKL_AST_LOC (asrt) = stmt_loc;

It is better to let the caller of this function to set the location of
the returned entity, like it is done in the ASSERT... parser rules.

> +
> +  return asrt;
> +}
> +
>  #if 0
>  /* Register a list of arguments in the compile-time environment.  This
>     is used by function specifiers and try-catch statements.
> @@ -1932,6 +2007,24 @@ simple_stmt:
>                      PKL_AST_LOC (PKL_AST_TYPE ($3)) = @3;
>                    PKL_AST_LOC ($$) = @$;
>                  }
> +        | ASSERT '(' expression ')'
> +                {
> +                  pkl_ast_node asrt = pkl_make_assertion (pkl_parser, $3, 
> NULL,
> +                                                          @$);
> +                  if (asrt == NULL)
> +                    YYERROR;
> +                  $$ = asrt;
> +                  PKL_AST_LOC ($$) = @$;
> +                }
> +        | ASSERT '(' expression ',' expression ')'
> +                {
> +                  pkl_ast_node asrt = pkl_make_assertion (pkl_parser, $3, $5,
> +                                                          @$);
> +                  if (asrt == NULL)
> +                    YYERROR;
> +                  $$ = asrt;
> +                  PKL_AST_LOC ($$) = @$;
> +                }

This is one of the few cases where I would pesonally use an assignment
in an expression, because it helps readability:

if (($$ = pkl_make_assertion (pkl_parser,...)) == NULL)
  YYERROR;
PKL_AST_LOC ($$) = @$;

But this is definitely a personal taste matter, so feel free to discard
this :)

> diff --git a/testsuite/poke.pkl/assert-4.pk b/testsuite/poke.pkl/assert-4.pk
> new file mode 100644
> index 00000000..b36313ad
> --- /dev/null
> +++ b/testsuite/poke.pkl/assert-4.pk
> @@ -0,0 +1,16 @@
> +/* { dg-do run } */
> +
> +fun a = (int cond) void:
> +  {
> +    assert (1 == 1, "One is equal to one");
> +
> +    try assert (cond); /* Line 7. Assert statement starts at column 9. */
> +    catch (Exception ex)
> +      {
> +        print (ex.msg + "\n");
> +      }
> +  }
> +
> +/* { dg-command { a (1) } } */
> +/* { dg-command { a (0) } } */
> +/* { dg-output "assertion failed at .*:7:9" } */

Excellent! :)