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

[Mohammad-Reza Nabipoor]

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.


> > +    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)
```

or,

```c
#define ASTREF(AST)    \
  ((AST) ? (++((AST)->common.refcount), ((AST) = (AST))) : NULL)
#define ASTDEREF(AST)  \
  ((AST) ? (--((AST)->common.refcount), ((AST) = (AST))) : NULL)
```

This way the compiler can help people like me who don't have a good memory!


> > +
> > +    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.
> 


You should blame `clang-format -style=gnu` for that :)


> > +
> > +  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.
> 


OK.


> > +        | 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 :)


I like this; it's shorter.



Regards,
Mohammad-Reza