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

[Jose E. Marchesi]

> Hi, Jose!
>
> On Thu, Nov 26, 2020 at 08:31:30AM +0100, Jose E. Marchesi wrote:
>> 
>> Thanks for the new version of the patch.
>
>
> Thanks for your review.
>
>
>> > +  /* 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.
>>
>
>
> Think about `assert ("hello")`.
> With this line you can have `^~~~~~~` in the error message:
>
> ```
> <stdin>:1:9: error: function argument 1 has the wrong type
> <stdin>:1:9: error: expected uint<64>, got string
> assert ("hello");
>         ^~~~~~~
> ```
>
>> > +
>> > +  /* 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.
>> 
>
>
> Likewise.
>

You are right!  I forgot you are basically synthetizing the message and
expression syntactic entities you get from the parser into a funcall.

So yeah, you definitely want these locs.

>> > +    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 ;)
>> 
>
>
> :facepalm:
>
> What about changing the `ASTREF` and `ASTDEREF` to:
>
> ```c
> #define ASTREF(AST)    \
>   ((AST) ? (((AST) = (AST)), ++((AST)->common.refcount), (AST)) : NULL)
> #define ASTDEREF(AST)  \
>   ((AST) ? (((AST) = (AST)), --((AST)->common.refcount), (AST)) : NULL)
> ```

This is a good trick.  But please make this change in a different patch.

The assert patch is OK for master!
Thanks.