Re: [PATCH] pkl: codegen: change assertion in stack pushes to error

[Jose E. Marchesi]

Hi Mohammad.

> 2023-01-02  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
>
>       * libpoke/pkl-gen.c (PKL_GEN_ASSERT): New macro.
>       (PKL_GEN_PUSH_AN_ASM): s/assert/PKL_GEN_ASSERT/.
>       (PKL_GEN_PUSH_CONTEXT): Likewise.
> ---
>
> Hi Jose.
>
> The motivation for this patch is to handle the following input
> more gracefully:
>
> ```poke
> vm_odepth>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U>0U;
> ```
>
> I kept the `assert` in `PKL_GEN_POP{AN_ASM,CONTEXT}` macros (because
> assertion failure in POP operation is certainly a bug in the
> compiler).
>
> I also decided to use PKL_ERROR instead of PKL_ICE.

That makes sense, but in the error message I would not talk about
assertions.  A compile-time error is an error.  What about something
like "maximum depth level exceeded in compiler"?

It would be also nice to provide a way for the user to set that maximum
depth level, maybe using a libpoke/dot-command interface?

> Although I'm not happy with the name `PKL_GEN_ASSERT`, do you have a
> better suggestion?

I would probably not use a macro at all in this case: it is by itself
bigger than the two current usages.

>
> Regards,
> Mohammad-Reza
>
>
>  ChangeLog         |  6 ++++++
>  libpoke/pkl-gen.c | 15 +++++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 26e229fd..6a0a366c 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2023-01-02  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
> +
> +     * libpoke/pkl-gen.c (PKL_GEN_ASSERT): New macro.
> +     (PKL_GEN_PUSH_AN_ASM): s/assert/PKL_GEN_ASSERT/.
> +     (PKL_GEN_PUSH_CONTEXT): Likewise.
> +
>  2023-01-01  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
>  
>       * testsuite/poke.map/maps-structs-constraints-2.pk: Fix expected
> diff --git a/libpoke/pkl-gen.c b/libpoke/pkl-gen.c
> index 6eeab3e7..849a8077 100644
> --- a/libpoke/pkl-gen.c
> +++ b/libpoke/pkl-gen.c
> @@ -41,10 +41,21 @@
>  #define PKL_GEN_ASM  PKL_GEN_AN_ASM(pasm)
>  #define PKL_GEN_ASM2 PKL_GEN_AN_ASM(pasm2)
>  
> +#define PKL_GEN_ASSERT(cond)                                              \
> +  do                                                                      \
> +    {                                                                     \
> +      if (cond)                                                           \
> +        break;                                                            \
> +      PKL_ERROR (PKL_AST_NOLOC, __FILE__ ":%d: assertion failed: " #cond, \
> +                 __LINE__);                                               \
> +      PKL_PASS_ERROR;                                                     \
> +    }                                                                     \
> +  while (0)
> +
>  #define PKL_GEN_PUSH_AN_ASM(ASM,new_pasm)                               \
>    do                                                                    \
>      {                                                                   \
> -      assert (PKL_GEN_PAYLOAD->cur_##ASM < PKL_GEN_MAX_PASM);           \
> +      PKL_GEN_ASSERT (PKL_GEN_PAYLOAD->cur_##ASM < PKL_GEN_MAX_PASM);   \
>        PKL_GEN_PAYLOAD->ASM[++(PKL_GEN_PAYLOAD->cur_##ASM)] = (new_pasm); \
>      }                                                                   \
>    while (0)
> @@ -69,7 +80,7 @@
>  #define PKL_GEN_PUSH_CONTEXT                                            \
>    do                                                                    \
>      {                                                                   \
> -      assert (PKL_GEN_PAYLOAD->cur_context < PKL_GEN_MAX_CTX);          \
> +      PKL_GEN_ASSERT (PKL_GEN_PAYLOAD->cur_context < PKL_GEN_MAX_CTX);  \
>        PKL_GEN_PAYLOAD->context[PKL_GEN_PAYLOAD->cur_context + 1]        \
>          = 0;                                                            \
>        PKL_GEN_PAYLOAD->cur_context++;                                   \