Re: [PATCH] pkl: trans: get rid of pkl_ast_finish_breaks

[Jose E. Marchesi]

Hi Mohammad.
Thanks for the patch.

On a general note, maybe "escapable" is better than "breakable" for
breakable/continuable entities?

> diff --git a/libpoke/pkl-trans.c b/libpoke/pkl-trans.c
> index 4d27cbe3..2503b253 100644
> --- a/libpoke/pkl-trans.c
> +++ b/libpoke/pkl-trans.c
> @@ -128,6 +128,30 @@ pkl_trans_in_functions (struct pkl_trans_function_ctx 
> functions[],
>      }                                                           \
>    while (0)
>  
> +#define PKL_TRANS_BREAKABLE                                     \
> +  (PKL_TRANS_PAYLOAD->next_breakable == 0                       \
> +   ? NULL                                                       \
> +   : &PKL_TRANS_PAYLOAD->breakables[PKL_TRANS_PAYLOAD->next_breakable - 1])
> +
> +#define PKL_TRANS_PUSH_BREAKABLE(b)                             \
> +  do                                                            \
> +    {                                                           \
> +      assert (PKL_TRANS_PAYLOAD->next_breakable < 
> PKL_TRANS_MAX_COMP_STMT_NEST);\
> +      
> PKL_TRANS_PAYLOAD->breakables[PKL_TRANS_PAYLOAD->next_breakable].nframes  \
> +        = 0;                                                    \
> +      
> PKL_TRANS_PAYLOAD->breakables[PKL_TRANS_PAYLOAD->next_breakable++].breakable \
> +        = (b);                                                  \

I would call the field `node' rather than `breakable'.  Like in the
stack of function contexts.

> +    }                                                           \
> +  while (0)
> +
> +#define PKL_TRANS_POP_BREAKABLE                                 \
> +  do                                                            \
> +    {                                                           \
> +      assert (PKL_TRANS_PAYLOAD->next_breakable > 0);           \
> +      PKL_TRANS_PAYLOAD->next_breakable--;                      \
> +    }                                                           \
> +  while (0)
> +
>  /* The following handler is used in all trans phases and initializes
>     the phase payload.  */
>  
> @@ -1095,6 +1119,17 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_pr_comp_stmt)
>        PKL_TRANS_FUNCTION->ndrops++;
>        PKL_TRANS_FUNCTION->npopes++;
>      }
> +
> +  if (PKL_TRANS_BREAKABLE)
> +    {
> +      PKL_TRANS_BREAKABLE->nframes++;
> +
> +      /* try {} catch (Exception e) { if_we_are_here; } */
> +      if (PKL_AST_CODE (PKL_PASS_PARENT) == PKL_AST_TRY_STMT
> +          && PKL_AST_TRY_STMT_ARG (PKL_PASS_PARENT)
> +          && PKL_AST_TRY_STMT_HANDLER (PKL_PASS_PARENT) == PKL_PASS_NODE)
> +        PKL_TRANS_BREAKABLE->nframes++;
> +    }
>  }

In this case I would much prefer to have this instead:

    : try_stmt
    : |
    : +-- [arg]
    : +-- [exp]
    : +-- try_stmt_body
    : |   |
    : |   +-- code
    : |
    : +-- try_stmt_handler
    :     |
    :     +-- code

instead of the PKL_AST_PARENT trick, which is only possible because we
previously moved the `body' into its own node.  Otherwise the solution
seems "incomplete" :)

>  PKL_PHASE_END_HANDLER
>  
> @@ -1121,6 +1156,8 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_pr_loop_stmt)
>  
>    if (PKL_TRANS_FUNCTION && PKL_AST_LOOP_STMT_ITERATOR (stmt))
>      PKL_TRANS_FUNCTION->ndrops += 3;
> +
> +  PKL_TRANS_PUSH_BREAKABLE (stmt);
>  }
>  PKL_PHASE_END_HANDLER
>  
> @@ -1135,6 +1172,8 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_loop_stmt)
>    if (PKL_TRANS_FUNCTION
>        && (PKL_TRANS_FUNCTION && PKL_AST_LOOP_STMT_ITERATOR (stmt)))
>      PKL_TRANS_FUNCTION->ndrops -= 3;
> +
> +  PKL_TRANS_POP_BREAKABLE;
>  }
>  PKL_PHASE_END_HANDLER
>  
> @@ -1169,6 +1208,17 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_comp_stmt)
>        PKL_TRANS_FUNCTION->ndrops--;
>        PKL_TRANS_FUNCTION->npopes--;
>      }
> +
> +  if (PKL_TRANS_BREAKABLE)
> +    {
> +      PKL_TRANS_BREAKABLE->nframes--;
> +
> +      /* try {} catch (Exception e) { if_we_are_here; } */
> +      if (PKL_AST_CODE (PKL_PASS_PARENT) == PKL_AST_TRY_STMT
> +          && PKL_AST_TRY_STMT_ARG (PKL_PASS_PARENT)
> +          && PKL_AST_TRY_STMT_HANDLER (PKL_PASS_PARENT) == PKL_PASS_NODE)
> +        PKL_TRANS_BREAKABLE->nframes--;
> +    }
>  }
>  PKL_PHASE_END_HANDLER
>  
> @@ -1220,6 +1270,9 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_pr_try_stmt_body)
>  {
>    if (PKL_TRANS_FUNCTION)
>      PKL_TRANS_FUNCTION->npopes++;
> +
> +  if (PKL_AST_TRY_STMT_KIND (PKL_PASS_PARENT) == PKL_AST_TRY_STMT_KIND_UNTIL)
> +    PKL_TRANS_PUSH_BREAKABLE (PKL_PASS_PARENT);
>  }
>  PKL_PHASE_END_HANDLER
>  
> @@ -1227,6 +1280,47 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_try_stmt_body)
>  {
>    if (PKL_TRANS_FUNCTION)
>      PKL_TRANS_FUNCTION->npopes--;
> +
> +  if (PKL_AST_TRY_STMT_KIND (PKL_PASS_PARENT) == PKL_AST_TRY_STMT_KIND_UNTIL)
> +    PKL_TRANS_POP_BREAKABLE;
> +}
> +PKL_PHASE_END_HANDLER
> +
> +PKL_PHASE_BEGIN_HANDLER (pkl_trans1_pr_try_stmt)
> +{
> +  if (PKL_AST_TRY_STMT_KIND (PKL_PASS_NODE) == PKL_AST_TRY_STMT_KIND_UNTIL)
> +    PKL_TRANS_PUSH_BREAKABLE (PKL_PASS_NODE);
> +}
> +PKL_PHASE_END_HANDLER
> +
> +PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_try_stmt)
> +{
> +  if (PKL_AST_TRY_STMT_KIND (PKL_PASS_NODE) == PKL_AST_TRY_STMT_KIND_UNTIL)
> +    PKL_TRANS_POP_BREAKABLE;
> +}
> +PKL_PHASE_END_HANDLER
> +
> +PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_break_stmt)
> +{
> +  if (PKL_TRANS_BREAKABLE)
> +    {
> +      PKL_AST_BREAK_STMT_ENTITY (PKL_PASS_NODE)
> +        = PKL_TRANS_BREAKABLE->breakable;
> +      PKL_AST_BREAK_STMT_NFRAMES (PKL_PASS_NODE)
> +        = PKL_TRANS_BREAKABLE->nframes;
> +    }
> +}
> +PKL_PHASE_END_HANDLER
> +
> +PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_continue_stmt)
> +{
> +  if (PKL_TRANS_BREAKABLE)
> +    {
> +      PKL_AST_CONTINUE_STMT_ENTITY (PKL_PASS_NODE)
> +        = PKL_TRANS_BREAKABLE->breakable;
> +      PKL_AST_CONTINUE_STMT_NFRAMES (PKL_PASS_NODE)
> +        = PKL_TRANS_BREAKABLE->nframes;
> +    }
>  }
>  PKL_PHASE_END_HANDLER
>  
> @@ -1250,6 +1344,10 @@ struct pkl_phase pkl_phase_trans1 =
>     PKL_PHASE_PS_HANDLER (PKL_AST_LOOP_STMT_ITERATOR, 
> pkl_trans1_ps_loop_stmt_iterator),
>     PKL_PHASE_PR_HANDLER (PKL_AST_LOOP_STMT, pkl_trans1_pr_loop_stmt),
>     PKL_PHASE_PS_HANDLER (PKL_AST_LOOP_STMT, pkl_trans1_ps_loop_stmt),
> +   PKL_PHASE_PS_HANDLER (PKL_AST_BREAK_STMT, pkl_trans1_ps_break_stmt),
> +   PKL_PHASE_PS_HANDLER (PKL_AST_CONTINUE_STMT, pkl_trans1_ps_continue_stmt),
> +   PKL_PHASE_PR_HANDLER (PKL_AST_TRY_STMT_BODY, pkl_trans1_pr_try_stmt),
> +   PKL_PHASE_PS_HANDLER (PKL_AST_TRY_STMT_BODY, pkl_trans1_ps_try_stmt),
>     PKL_PHASE_PR_HANDLER (PKL_AST_TRY_STMT_BODY, pkl_trans1_pr_try_stmt_body),
>     PKL_PHASE_PS_HANDLER (PKL_AST_TRY_STMT_BODY, pkl_trans1_ps_try_stmt_body),
>     PKL_PHASE_PR_HANDLER (PKL_AST_STRUCT_TYPE_FIELD, 
> pkl_trans1_pr_struct_type_field),
> diff --git a/libpoke/pkl-trans.h b/libpoke/pkl-trans.h
> index 8d3b1f6f..8e470368 100644
> --- a/libpoke/pkl-trans.h
> +++ b/libpoke/pkl-trans.h
> @@ -53,6 +53,23 @@ struct pkl_trans_function_ctx
>    struct pkl_trans_function_ctx *next;
>  };
>  
> +/* The trans phases keep a stack of breakable/continuable entities (loops
> +   and try-until statements) and corresponding `break' and `continue'
> +   keywords, while they operate on the AST.
> +
> +   The following struct implements each entry on the breakable/continuable
> +   entities stack.
> +
> +   BREAKABLE is the breakable/continuable entity (loops and try-until).
> +
> +   NFRAMES is the number of frames in the body of BREAKABLE.  */
> +
> +struct pkl_trans_breakable_ctx
> +{
> +  pkl_ast_node breakable;
> +  int nframes;
> +};
> +
>  /* The following struct defines the payload of the trans phases.
>  
>     ERRORS is the number of errors detected while running the phase.
> @@ -69,10 +86,16 @@ struct pkl_trans_function_ctx
>     when mapping and writing integral types.
>  
>     CUR_ENDIAN is the index to ENDIAN and marks the top of the stack of
> -   endianness.  Initially PKL_AST_ENDIAN_DFL.  */
> +   endianness.  Initially PKL_AST_ENDIAN_DFL.
> +
> +   BREAKABLES is a stack of breakables.
> +
> +   NEXT_BREAKABLE - 1 is the index for the enclosing breakable
> +   entity.  */
>  
>  #define PKL_TRANS_MAX_FUNCTION_NEST 32
>  #define PKL_TRANS_MAX_ENDIAN 25
> +#define PKL_TRANS_MAX_COMP_STMT_NEST 32
>  
>  struct pkl_trans_payload
>  {
> @@ -82,6 +105,8 @@ struct pkl_trans_payload
>    int next_function;
>    enum pkl_ast_endian endian[PKL_TRANS_MAX_ENDIAN];
>    int cur_endian;
> +  struct pkl_trans_breakable_ctx breakables[PKL_TRANS_MAX_COMP_STMT_NEST];
> +  int next_breakable;
>  };
>  
>  typedef struct pkl_trans_payload *pkl_trans_payload;