Re: [PATCH v2 1/2] libpoke: Add new getter and setters for pk_vals

[Jose E. Marchesi]

Hi Mohammad.

This looks great!

My only comment is: aren't we supposed to update pkc->status in every
call to libpoke?  In most (all?) of the new functions you added I think
this is enough:

  pkc->status = PK_OK;

But please double-check :)

Other than that, this is OK for master.

> 2021-03-28  Mohammad-Reza Nabipoor  <m.nabipoor@yahoo.com>
>
>       * libpoke/libpoke.h (pk_val_mappable_p): New function declaration.
>       (pk_val_set_mapped): Likewise.
>       (pk_val_strict_p): Likewise.
>       (pk_val_set_strict): Likewise.
>       (pk_val_set_ios): Likewise.
>       (pk_val_set_offset): Likewise.
>       (pk_val_boffset): Likewise.
>       (pk_val_set_boffset): Likewise.
>       * libpoke/pvm-val.h (PVM_VAL_MAPPABLE_P): New macro.
>       * libpoke/pk-val.c (pk_val_mappable_p): New function definition.
>       (pk_val_set_mapped): Likewise.
>       (pk_val_strict_p): Likewise.
>       (pk_val_set_strict): Likewise.
>       (pk_val_set_ios): Likewise.
>       (pk_val_set_offset): Likewise.
>       (pk_val_boffset): Likewise.
>       (pk_val_set_boffset): Likewise.
>       * testsuite/poke.libpoke/values.c (test_simple_values_mapping):
>       New tests.
> ---
>  ChangeLog                       | 22 ++++++++++++
>  libpoke/libpoke.h               | 56 +++++++++++++++++++++++++++--
>  libpoke/pk-val.c                | 59 ++++++++++++++++++++++++++++++
>  libpoke/pvm-val.h               |  2 ++
>  testsuite/poke.libpoke/values.c | 64 +++++++++++++++++++++++++++++++++
>  5 files changed, 201 insertions(+), 2 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 505365fa..ff49f4fd 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,25 @@
> +2021-03-28  Mohammad-Reza Nabipoor  <m.nabipoor@yahoo.com>
> +
> +     * libpoke/libpoke.h (pk_val_mappable_p): New function declaration.
> +     (pk_val_set_mapped): Likewise.
> +     (pk_val_strict_p): Likewise.
> +     (pk_val_set_strict): Likewise.
> +     (pk_val_set_ios): Likewise.
> +     (pk_val_set_offset): Likewise.
> +     (pk_val_boffset): Likewise.
> +     (pk_val_set_boffset): Likewise.
> +     * libpoke/pvm-val.h (PVM_VAL_MAPPABLE_P): New macro.
> +     * libpoke/pk-val.c (pk_val_mappable_p): New function definition.
> +     (pk_val_set_mapped): Likewise.
> +     (pk_val_strict_p): Likewise.
> +     (pk_val_set_strict): Likewise.
> +     (pk_val_set_ios): Likewise.
> +     (pk_val_set_offset): Likewise.
> +     (pk_val_boffset): Likewise.
> +     (pk_val_set_boffset): Likewise.
> +     * testsuite/poke.libpoke/values.c (test_simple_values_mapping):
> +     New tests.
> +
>  2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
>  
>       * testsuite/poke.map/ass-map-20.pk: New test.
> diff --git a/libpoke/libpoke.h b/libpoke/libpoke.h
> index 38866718..4aa1fb1e 100644
> --- a/libpoke/libpoke.h
> +++ b/libpoke/libpoke.h
> @@ -875,21 +875,73 @@ pk_val pk_array_type_bound (pk_val type) LIBPOKE_API;
>  
>  /* Mapped values.  */
>  
> +/* Return a boolean indicating whether the given value is mappable or
> +   not.  */
> +
> +int pk_val_mappable_p (pk_val val) LIBPOKE_API;
> +
>  /* Return a boolean indicating whether the given value is mapped or
>     not.  */
>  
>  int pk_val_mapped_p (pk_val val) LIBPOKE_API;
>  
> +/* Set MAPPED_P flag of the given value indicating whether the given
> +   value is mapped or not.
> +
> +   For non-mappable values, this is a no-operation.  Note that this
> +   just sets the flag.  */
> +
> +void pk_val_set_mapped (pk_val val, int mapped_p) LIBPOKE_API;
> +
> +/* Return a boolean indicating whether data integrity for the given
> +   value is enforced or not.  */
> +
> +int pk_val_strict_p (pk_val val) LIBPOKE_API;
> +
> +/* Set STRICT flag of the given value indicating whether data
> +   integrity is enforced or not.
> +
> +   For non-mappable values, this is a no-operation.  Note that this
> +   just sets the flag.  */
> +
> +void pk_val_set_strict (pk_val val, int strict_p) LIBPOKE_API;
> +
>  /* Return the IOS identifier, an int<32>, in which the given value is
>     mapped.  If the value is not mapped, return PK_NULL.  */
>  
>  pk_val pk_val_ios (pk_val val) LIBPOKE_API;
>  
> +/* Set the IOS, an int<32>, in which the given value is mapped.
> +
> +   For non-mappable values, this is a no-operation.  Note that this
> +   just sets the IOS property.  */
> +
> +void pk_val_set_ios (pk_val val, pk_val ios) LIBPOKE_API;
> +
>  /* Return the offset in which the given value is mapped.
>     If the value is not mapped, return PK_NULL.  */
>  
> -pk_val pk_val_offset
> -(pk_val val) LIBPOKE_API;
> +pk_val pk_val_offset (pk_val val) LIBPOKE_API;
> +
> +/* Set the offset in which the given value is mapped.
> +
> +   For non-mappable values, this is a no-operation.  Note that this
> +   just sets the property.  */
> +
> +void pk_val_set_offset (pk_val val, pk_val offset) LIBPOKE_API;
> +
> +/* Return the bit offset in which the given value is mapped.
> +   If the value is not mapped, return PK_NULL.  */
> +
> +pk_val pk_val_boffset (pk_val val) LIBPOKE_API;
> +
> +/* Set the bit offset in which the given value is mapped.
> +   BOFFSET is expected to be an uint<64>.
> +
> +   For non-mappable values, this is a no-operation.  Note that this
> +   just sets the property.  */
> +
> +void pk_val_set_boffset (pk_val val, pk_val boffset) LIBPOKE_API;
>  
>  /* Other operations on values.  */
>  
> diff --git a/libpoke/pk-val.c b/libpoke/pk-val.c
> index 2f530235..30a9ba5b 100644
> --- a/libpoke/pk-val.c
> +++ b/libpoke/pk-val.c
> @@ -130,18 +130,49 @@ pk_offset_unit (pk_val val)
>    return PVM_VAL_OFF_UNIT (val);
>  }
>  
> +int
> +pk_val_mappable_p (pk_val val)
> +{
> +  return PVM_VAL_MAPPABLE_P (val);
> +}
> +
>  int
>  pk_val_mapped_p (pk_val val)
>  {
>    return PVM_VAL_MAPPED_P (val);
>  }
>  
> +void
> +pk_val_set_mapped (pk_val val, int mapped_p)
> +{
> +  PVM_VAL_SET_MAPPED_P (val, !!mapped_p);
> +}
> +
> +int
> +pk_val_strict_p (pk_val val)
> +{
> +  return PVM_VAL_STRICT_P (val);
> +}
> +
> +void
> +pk_val_set_strict (pk_val val, int strict_p)
> +{
> +  PVM_VAL_SET_STRICT_P (val, !!strict_p);
> +}
> +
>  pk_val
>  pk_val_ios (pk_val val)
>  {
>    return PVM_VAL_IOS (val);
>  }
>  
> +void
> +pk_val_set_ios (pk_val val, pk_val ios)
> +{
> +  if (PVM_IS_INT (ios) && PVM_VAL_INT_SIZE (ios) == 32)
> +    PVM_VAL_SET_IOS (val, ios);
> +}
> +
>  pk_val
>  pk_val_offset (pk_val val)
>  {
> @@ -165,6 +196,34 @@ pk_val_offset (pk_val val)
>                              pvm_make_ulong (1, 64));
>  }
>  
> +void
> +pk_val_set_offset (pk_val val, pk_val off)
> +{
> +  uint64_t boff;
> +
> +  if (!PVM_IS_OFF (off))
> +    return;
> +
> +  boff = PVM_VAL_INTEGRAL (PVM_VAL_OFF_MAGNITUDE (off))
> +         * PVM_VAL_ULONG (PVM_VAL_OFF_UNIT (off));
> +  PVM_VAL_SET_OFFSET (val, pvm_make_ulong (boff, 64));
> +}
> +
> +pk_val
> +pk_val_boffset (pk_val val)
> +{
> +  pvm_val val_boffset = PVM_VAL_OFFSET (val);
> +
> +  return val_boffset == PVM_NULL ? PK_NULL : val_boffset;
> +}
> +
> +void
> +pk_val_set_boffset (pk_val val, pk_val boff)
> +{
> +  if (PVM_IS_ULONG (boff) && PVM_VAL_ULONG_SIZE (boff) == 64)
> +    PVM_VAL_SET_OFFSET (val, boff);
> +}
> +
>  int
>  pk_type_code (pk_val val)
>  {
> diff --git a/libpoke/pvm-val.h b/libpoke/pvm-val.h
> index 9c265f80..41801d16 100644
> --- a/libpoke/pvm-val.h
> +++ b/libpoke/pvm-val.h
> @@ -584,6 +584,8 @@ typedef struct pvm_off *pvm_off;
>     : PVM_IS_ULONG ((V)) ? PVM_VAL_ULONG ((V))    \
>     : 0)
>  
> +#define PVM_VAL_MAPPABLE_P(V) (PVM_IS_ARR ((V)) || PVM_IS_SCT ((V)))
> +
>  /* The following macros allow to handle map-able PVM values (such as
>     arrays and structs) polymorphically.
>  
> diff --git a/testsuite/poke.libpoke/values.c b/testsuite/poke.libpoke/values.c
> index 6d94eded..f3a9b8f0 100644
> --- a/testsuite/poke.libpoke/values.c
> +++ b/testsuite/poke.libpoke/values.c
> @@ -154,6 +154,69 @@ test_simple_values ()
>  #undef T
>  }
>  
> +void
> +test_simple_values_mapping ()
> +{
> +  pk_val simple_values[] = {
> +    pk_make_int (1, 23),
> +    pk_make_int (2, 46),
> +    pk_make_uint (3, 23),
> +    pk_make_uint (4, 46),
> +    pk_make_string ("Poke"),
> +    pk_make_offset (pk_make_uint (5, 64), pk_make_uint (6, 64)),
> +  };
> +  const int N = sizeof (simple_values) / sizeof (simple_values[0]);
> +  pk_val i32 = pk_make_int (7, 32);
> +  pk_val u64 = pk_make_uint (8, 64);
> +
> +/* test case */
> +#define T0(cond, ...)                                                        
>  \
> +  do                                                                         
>  \
> +    {                                                                        
>  \
> +      if (cond)                                                              
>  \
> +        pass (__VA_ARGS__);                                                  
>  \
> +      else                                                                   
>  \
> +        fail (__VA_ARGS__);                                                  
>  \
> +    }                                                                        
>  \
> +  while (0)
> +
> +#define T(i, cond) T0 ((cond), "[%s:%d] i:%d " #cond, __func__, __LINE__, 
> (i))
> +
> +  T0 (i32 != PK_NULL, "i32 != PK_NULL");
> +  T0 (u64 != PK_NULL, "u64 != PK_NULL");
> +  for (int i = 0; i < N; i++)
> +    T (i, simple_values[i] != PK_NULL);
> +
> +  for (int i = 0; i < N; i++)
> +    {
> +      T (i, pk_val_mappable_p (simple_values[i]) == 0);
> +
> +      /* getters */
> +      T (i, pk_val_ios (simple_values[i]) == PK_NULL);
> +      T (i, pk_val_boffset (simple_values[i]) == PK_NULL);
> +      T (i, pk_val_offset (simple_values[i]) == PK_NULL);
> +      T (i, pk_val_mapped_p (simple_values[i]) == 0);
> +      T (i, pk_val_strict_p (simple_values[i]) == 0);
> +
> +      /* setters: no-op for simple values! */
> +      pk_val_set_ios (simple_values[i], i32);
> +      pk_val_set_boffset (simple_values[i], u64);
> +      pk_val_set_offset (simple_values[i], u64);
> +      pk_val_set_mapped (simple_values[i], 1);
> +      pk_val_set_strict (simple_values[i], 1);
> +
> +      /* re-check to confirm setters don't work on simple values */
> +      T (i, pk_val_ios (simple_values[i]) == PK_NULL);
> +      T (i, pk_val_boffset (simple_values[i]) == PK_NULL);
> +      T (i, pk_val_offset (simple_values[i]) == PK_NULL);
> +      T (i, pk_val_mapped_p (simple_values[i]) == 0);
> +      T (i, pk_val_strict_p (simple_values[i]) == 0);
> +    }
> +
> +#undef T
> +#undef T0
> +}
> +
>  #define STARTS_WITH(PREFIX, STR) (strncmp (PREFIX, STR, strlen (PREFIX)) == 
> 0)
>  
>  void
> @@ -266,6 +329,7 @@ int
>  main (int argc, char *argv[])
>  {
>    test_simple_values ();
> +  test_simple_values_mapping ();
>    test_pk_val_equal_p ();
>  
>    totals ();