Re: [PATCH] libpoke,etc: Re-write `ASTREF`/`ASTDEREF` as an inline function

[Jose E. Marchesi]

Hi Mohammad.

> diff --git a/HACKING b/HACKING
> index 41e6bc21..d11f2c80 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -1,9 +1,9 @@
> -                       __________________________
> +                    __________________________
>  
> -                        GNU POKE - HACKING NOTES
> +                     GNU POKE - HACKING NOTES
>  
> -                            The poke hackers
> -                       __________________________
> +                         The poke hackers
> +                    __________________________
>  

This looks like your org-mode changed theheader slightly.
Its ok.

>  Table of Contents
> @@ -1743,7 +1743,7 @@ with GNU poke.  If not, see 
> <https://www.gnu.org/licenses/>.
>  
>    The AST uses reference counting in order to manage the memory used by
>    the nodes.  Every time you store a pointer to an AST node, you should
> -  use the macro `ASTREF' in order to increase its counter:
> +  use the `ASTREF' in order to increase its counter:

"use the `ASTREF' function" ?

>    ,----
>    | pkl_ast_node foo = ASTREF (node);
> @@ -1758,27 +1758,15 @@ with GNU poke.  If not, see 
> <https://www.gnu.org/licenses/>.
>    `----
>  
>  
> -  There is a little caveat: the way ASTREF is defined, it requires a
> -  l-value to work properly.  Therefore, this doesn't work:
> -
> -  ,----
> -  | pkl_ast_node foo = ASTREF (PKL_AST_TYPE (node));
> -  `----
> -
> -
> -  instead, write:
> -
> -  ,----
> -  | pkl_ast_node type = PKL_AST_TYPE (node);
> -  | pkl_ast_node foo = ASTREF (type);
> -  `----
> +  To make sure you get the reference counting right, it's required to
> +  use the returned value of `ASTREF'.

What about adding there: "The compiler will warn you otherwise."

>  
>  13.2 Using ASTDEREF
>  ~~~~~~~~~~~~~~~~~~~
>  
> -  `ASTDEREF' decreases the reference counter of the provided AST node.
> -  The passed value should be a l-value.
> +  `ASTDEREF' decreases the reference counter of the provided AST
> +  node. It's required to use the returned value of `ASTDEREF'.
>  
>    In practice you will seldom find yourself in the need to use
>    `ASTDEREF'.  Just make sure that every `ASTREF' is paired with a

Ditto for ASTDEREF.

> diff --git a/etc/hacking.org b/etc/hacking.org
> index 88160deb..4d5e5158 100644
> --- a/etc/hacking.org
> +++ b/etc/hacking.org
> @@ -1369,9 +1369,10 @@ Each pickle in =pickles/FOO.pk= shall have a testsuite 
> in
>  
>  ** Using ASTREF
>  
> +
>     The AST uses reference counting in order to manage the memory used
>     by the nodes.  Every time you store a pointer to an AST node, you
> -   should use the macro =ASTREF= in order to increase its counter:
> +   should use the =ASTREF= in order to increase its counter:

the =ASTREF= function.

> diff --git a/libpoke/pkl-ast.h b/libpoke/pkl-ast.h
> index f1f7f664..333f5d69 100644
> --- a/libpoke/pkl-ast.h
> +++ b/libpoke/pkl-ast.h
> @@ -226,12 +226,6 @@ typedef union pkl_ast_node *pkl_ast_node;
>  #define PKL_AST_REGISTERED_P(AST) ((AST)->common.registered_p)
>  #define PKL_AST_REFCOUNT(AST) ((AST)->common.refcount)
>  
> -/* NOTE: both ASTREF and ASTDEREF need an l-value!  */
> -#define ASTREF(AST) ((AST) ? (++((AST)->common.refcount), (AST)) \
> -                     : NULL)
> -#define ASTDEREF(AST) ((AST) ? (--((AST)->common.refcount), (AST)) \
> -                       : NULL)
> -
>  struct pkl_ast_loc
>  {
>    int first_line;
> @@ -1894,6 +1888,22 @@ union pkl_ast_node
>    struct pkl_ast_print_stmt_arg print_stmt_arg;
>  };
>  
> +static inline pkl_ast_node __attribute__ ((always_inline, 
> warn_unused_result))
> +ASTREF (pkl_ast_node ast)
> +{
> +  if (ast)
> +    ++ast->common.refcount;
> +  return ast;
> +}
> +
> +static inline pkl_ast_node __attribute__ ((always_inline, 
> warn_unused_result))
> +ASTDEREF (pkl_ast_node ast)
> +{
> +  if (ast)
> +    --ast->common.refcount;
> +  return ast;
> +}
> +

Hm, I'm unsure regarding warn_unused_result and the next thunks in your
patch.  Sometimes ASTREF/ASTDEREF are called only for their side effect,
as you have found. Forcing using an assignment in these cases is a bit
ugly...

>  /* The `pkl_ast' struct defined below contains a PKL abstract syntax tree.
>  
>     AST contains the tree of linked nodes, starting with a
> diff --git a/libpoke/pkl-gen.c b/libpoke/pkl-gen.c
> index d929aa4b..7b31eb38 100644
> --- a/libpoke/pkl-gen.c
> +++ b/libpoke/pkl-gen.c
> @@ -3014,7 +3014,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_gen_pr_type_struct)
>          PKL_GEN_PAYLOAD->in_constructor = 1;
>  
>          pkl_asm_label (PKL_GEN_ASM, label);
> -        ASTREF(s); pkl_ast_node_free (s);
> +        s = ASTREF(s); pkl_ast_node_free (s);
>        }
>  
>        if (type_struct_constructor != PVM_NULL)
> diff --git a/libpoke/pkl-pass.c b/libpoke/pkl-pass.c
> index c1d2bb1e..8364bc2d 100644
> --- a/libpoke/pkl-pass.c
> +++ b/libpoke/pkl-pass.c
> @@ -616,7 +616,7 @@ pkl_do_pass_1 (pkl_compiler compiler,
>       this function will be stored in some other node (or the top-level
>       AST structure).  */
>    if (node != node_orig)
> -    ASTREF (node);
> +    node = ASTREF (node);
>  
>   _exit:
>    if (level != 0
> diff --git a/libpoke/pkl-promo.c b/libpoke/pkl-promo.c
> index 41577655..1479f0c5 100644
> --- a/libpoke/pkl-promo.c
> +++ b/libpoke/pkl-promo.c
> @@ -63,7 +63,7 @@ promote_integral (pkl_ast ast,
>            PKL_AST_TYPE (*a) = ASTREF (desired_type);
>            PKL_AST_LOC (*a) = loc;
>            PKL_AST_LOC (desired_type) = loc;
> -          ASTREF (*a);
> +          *a = ASTREF (*a);
>            *restart = 1;
>          }
>  
> @@ -125,7 +125,7 @@ promote_offset (pkl_ast ast,
>            *a = pkl_ast_make_cast (ast, type, ASTDEREF (*a));
>            PKL_AST_TYPE (*a) = ASTREF (type);
>            PKL_AST_LOC (*a) = loc;
> -          ASTREF (*a);
> +          *a = ASTREF (*a);
>            *restart = 1;
>          }
>  
> @@ -183,7 +183,7 @@ promote_array (pkl_ast ast,
>      *a = pkl_ast_make_cast (ast, type, ASTDEREF (*a));
>      PKL_AST_TYPE (*a) = ASTREF (type);
>      PKL_AST_LOC (*a) = loc;
> -    ASTREF (*a);
> +    *a = ASTREF (*a);
>      *restart = 1;
>      return 1;
>    }
> @@ -257,7 +257,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_promo_ps_op_div)
>  
>          pkl_ast_node unit_bit = pkl_ast_make_integer (PKL_PASS_AST, 1);
>  
> -        ASTREF (unit_bit);
> +        unit_bit = ASTREF (unit_bit);
>          PKL_AST_LOC (unit_bit) = PKL_AST_LOC (exp);
>  
>          if (!promote_offset (PKL_PASS_AST,
> @@ -597,7 +597,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_promo_ps_op_rela)
>                          && PKL_AST_TYPE_I_SIGNED_P (op2_base_type));
>  
>          pkl_ast_node unit_bit = pkl_ast_make_integer (PKL_PASS_AST, 1);
> -        ASTREF (unit_bit);
> +        unit_bit = ASTREF (unit_bit);
>          PKL_AST_LOC (unit_bit) = PKL_AST_LOC (exp);
>  
>          if (!promote_offset (PKL_PASS_AST,
> @@ -878,7 +878,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_promo_ps_type_array)
>        {
>          pkl_ast_node unit_bit = pkl_ast_make_integer (PKL_PASS_AST, 1);
>  
> -        ASTREF (unit_bit);
> +        unit_bit = ASTREF (unit_bit);
>          PKL_AST_LOC (unit_bit) = PKL_AST_LOC (PKL_PASS_NODE);
>  
>          if (!promote_offset (PKL_PASS_AST,
> @@ -1385,7 +1385,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_promo_ps_map)
>  
>    pkl_ast_node unit_bit = pkl_ast_make_integer (PKL_PASS_AST, 1);
>  
> -  ASTREF (unit_bit);
> +  unit_bit = ASTREF (unit_bit);
>    PKL_AST_LOC (unit_bit) = PKL_AST_LOC (PKL_PASS_NODE);
>  
>    if (!promote_offset (PKL_PASS_AST,
> @@ -1614,7 +1614,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_promo_ps_struct_type_field)
>                  PKL_PASS_ERROR;
>                }
>  
> -            ASTREF (unit_bit); pkl_ast_node_free (unit_bit);
> +            unit_bit = ASTREF (unit_bit); pkl_ast_node_free (unit_bit);
>              break;
>            }
>          default:
> diff --git a/libpoke/pkl-tab.y b/libpoke/pkl-tab.y
> index 14e26f78..bcd9d14f 100644
> --- a/libpoke/pkl-tab.y
> +++ b/libpoke/pkl-tab.y
> @@ -1434,7 +1434,7 @@ offset_type_specifier:
>                                                   $2,
>                                                   PKL_AST_DECL_INITIAL 
> (decl));
>  
> -                  ASTREF ($4); pkl_ast_node_free ($4);
> +                  $4 = ASTREF ($4); pkl_ast_node_free ($4);
>                    PKL_AST_LOC ($$) = @$;
>                  }
>          | OFFSETCONSTR simple_type_specifier ',' INTEGER '>'
> @@ -1724,7 +1724,7 @@ struct_type_field:
>                      {
>                        PKL_AST_LOC ($3) = @3;
>                        PKL_AST_TYPE ($3) = pkl_ast_make_string_type 
> (pkl_parser->ast);
> -                      ASTREF (PKL_AST_TYPE ($3));
> +                      PKL_AST_TYPE ($3) = ASTREF (PKL_AST_TYPE ($3));
>                        PKL_AST_LOC (PKL_AST_TYPE ($3)) = @3;
>                      }
>                  }
> @@ -2220,7 +2220,7 @@ stmt:
>                    PKL_AST_LOC ($$) = @$;
>  
>                    /* Free the identifier.  */
> -                  ASTREF ($3); pkl_ast_node_free ($3);
> +                  $3 = ASTREF ($3); pkl_ast_node_free ($3);
>  
>                    /* Annotate the contained BREAK and CONTINUE
>                       statements with their lexical level within this
> diff --git a/libpoke/pkl-typify.c b/libpoke/pkl-typify.c
> index fecccc92..c21fa07a 100644
> --- a/libpoke/pkl-typify.c
> +++ b/libpoke/pkl-typify.c
> @@ -2583,7 +2583,7 @@ PKL_PHASE_BEGIN_HANDLER 
> (pkl_typify1_ps_struct_type_field)
>            PKL_PASS_ERROR;
>          }
>  
> -      ASTREF (bool_type); pkl_ast_node_free (bool_type);
> +      bool_type = ASTREF (bool_type); pkl_ast_node_free (bool_type);
>      }
>  
>    /* Ditto for the optcond.  */
> @@ -2603,7 +2603,7 @@ PKL_PHASE_BEGIN_HANDLER 
> (pkl_typify1_ps_struct_type_field)
>            PKL_PASS_ERROR;
>          }
>  
> -      ASTREF (bool_type); pkl_ast_node_free (bool_type);
> +      bool_type = ASTREF (bool_type); pkl_ast_node_free (bool_type);
>      }
>  
>    /* If specified, the type of the initializer of a struct field
> @@ -2653,7 +2653,7 @@ expected %s, got %s",
>            PKL_PASS_ERROR;
>          }
>  
> -      ASTREF (offset_type); pkl_ast_node_free (offset_type);
> +      offset_type = ASTREF (offset_type); pkl_ast_node_free (offset_type);
>      }
>  }
>  PKL_PHASE_END_HANDLER