Re: [PATCH] pk_mi_json convert functions between a pk_val and a json_object.

[Jose E. Marchesi]

Hi Kostas.

    Hello! These are the functions that convert a pk_val to json_object
    and a json_object to the corresponding pk_val.

Thanks for the patch.
See comments below.
    
    From: kostasch <sdi1600195@di.uoa.gr>
    Date: Wed, 15 Jul 2020 00:04:24 +0300
    Subject: [PATCH] Added convert functions between a pk_val and a
     json_object.
    
    2020-07-14  Kostas Chasialis  <sdi1600195@di.uoa.gr>
    
        * poke/pk-mi-json.h (pk_mi_val_to_json): Prototype.
        (pk_mi_json_to_val): Prototype.
        * poke/pk-mi-json.c (pk_mi_val_to_json): Define.
        (pk_mi_json_to_val): Define.

pk_mi_val_to_json is also a new function isnt it?

        (_pk_mi_val_to_json): New function.
        (_pk_mi_make_json_int): Likewise.
        (_pk_mi_make_json_string): Likewise.
        (_pk_mi_make_json_offset): Likewise.
        (_pk_mi_make_json_struct): Likewise.
        (_pk_mi_make_json_array): Likewise.
        (_pk_mi_make_json_null): Likewise.
        (_pk_mi_json_to_val): Likewise.
        (_pk_mi_make_pk_int): Likewise.
        (_pk_mi_make_pk_uint): Likewise.
        (_pk_mi_make_pk_string): Likewise.
        (_pk_mi_make_pk_offset): Likewise.
        (_pk_mi_make_pk_struct): Likewise.
        (_pk_mi_make_pk_array): Likewise.
        (_pk_mi_get_json_poke_value_type): Likewise.
        (_pk_mi_json_array_element_pair): Likewise.

    diff --git a/poke/pk-mi-json.c b/poke/pk-mi-json.c
    index 3ec81924..8df1d572 100644
    --- a/poke/pk-mi-json.c
    +++ b/poke/pk-mi-json.c
    @@ -17,7 +17,10 @@
      */
     
     #include <config.h>
     #include <assert.h>
     #include <string.h>
     #include <json.h>
    @@ -429,6 +432,9 @@ pk_mi_json_to_msg (const char *str)
         return NULL;
     
       json = json_tokener_parse_ex (tokener, str, strlen (str));
    +  if (!json)
    +    pk_printf ("internal error: %s\n",
    +               json_tokener_error_desc (json_tokener_get_error (tokener)));
       json_tokener_free (tokener);

This is a spurious change, that indicates you need to rebase your branch
with the latest master.
     
       if (json)
    @@ -437,3 +443,620 @@ pk_mi_json_to_msg (const char *str)
       /* XXX: free the json object  */
       return msg;
     }
    +
    +/*Functions to convert pk_val to JSON Poke Value*/
    +static json_object* _pk_mi_make_json_int (pk_val integer, char **errmsg);
    +static json_object* _pk_mi_make_json_string (pk_val str, char **errmsg);
    +static json_object* _pk_mi_make_json_offset (pk_val offset, char **errmsg);
    +static json_object* _pk_mi_make_json_struct (pk_val sct, char **errmsg);
    +static json_object* _pk_mi_make_json_array (pk_val array, char **errmsg);
    +static json_object* _pk_mi_make_json_null ();

Please do not add declarations of the static functions, unless strictly
necessary, i.e. unless there is circular use.  Just define the function
before it is used.

Also, please do not prepend the names of the static functions with _.
There is no need to do that.

I find the names confusing.  These functions are supposed to convert
values to json, so why not pk_mi_int_to_json, pk_mi_string_to_json, etc?

    +static json_object* _pk_mi_val_to_json (pk_val val, char **errmsg)
    +{
    +  struct json_object *pk_val_object = NULL;

Leave an empty line between the declaration of automatics and the body
of the function.

    +  if  (val == PK_NULL) {

Open brace in a new line. Please use GNU style in your contributions.
There are many instances of this in the patch.

    +    pk_val_object = _pk_mi_make_json_null (errmsg);
    +  }
    +  else {
    +    switch (pk_type_code (pk_typeof (val))) {
    +      case PK_INT:
    +        pk_val_object = _pk_mi_make_json_int (val, errmsg);
    +        break;
    +      case PK_STRING:
    +        pk_val_object = _pk_mi_make_json_string (val, errmsg);
    +        break;
    +      case PK_OFFSET:
    +        pk_val_object = _pk_mi_make_json_offset (val, errmsg);
    +        break;
    +      case PK_STRUCT:
    +        pk_val_object = _pk_mi_make_json_struct (val, errmsg);
    +        break;
    +      case PK_ARRAY:
    +        pk_val_object = _pk_mi_make_json_array (val, errmsg);
    +        break;
    +      case PK_CLOSURE:
    +      case PK_ANY:
    +        assert (0);
    +    }
    +    if (!pk_val_object) {
    +      return NULL;
    +    }
    +  }
    +
    +  return pk_val_object;
    +}
    +
    +static json_object *
    +_pk_mi_make_json_int (pk_val integer, char **errmsg)
    +{
    +  struct json_object *int_object, *type_object, *value_object, 
*size_object;
    +  const char *type;
    +  int size;
    +
    +  assert (pk_type_code (pk_typeof (integer)) == PK_INT);
    +
    +  PK_MI_CHECK (errmsg, (int_object = json_object_new_object ()) != NULL, 
"json_object_new_object () failed");

Line too long.  There are many instances of long lines in the patch.
Please split them.

    +
    +  if  (! (pk_uint_value (pk_integral_type_signed_p (pk_typeof 
(integer))))) {
    +    PK_MI_CHECK (errmsg, (value_object = json_object_new_uint64 
(pk_uint_value (integer))) != NULL, "json_object_new_object () failed");
    +    size = pk_uint_size (integer);
    +    type = "UnsignedInteger";
    +  }
    +  else {
    +    PK_MI_CHECK (errmsg,  (value_object = json_object_new_int64 
(pk_int_value (integer))) != NULL, "json_object_new_object () failed");
    +    size = pk_int_size (integer);
    +    type = "Integer";
    +  }
    +
    +  assert (size <= 64);

Why an assert on size?  The size of an integer pk_val is guaranteed to
be <= 64.

    +
    +  PK_MI_CHECK (errmsg, (size_object = json_object_new_int (size)) != NULL, 
"json_object_new_object () failed");
    +  PK_MI_CHECK (errmsg, (type_object = json_object_new_string (type)) != 
NULL, "json_object_new_object () failed");
    +
    +  /*OK, fill the properties of our object*/

Please leave spaces in the comment, like this:

/* OK, file the properties of our object.  */

(Note the double space after .)

    diff --git a/poke/pk-mi-json.h b/poke/pk-mi-json.h
    index 57898c41..0a80150c 100644
    --- a/poke/pk-mi-json.h
    +++ b/poke/pk-mi-json.h
    @@ -22,6 +22,22 @@
     #include <config.h>
     
     #include "pk-mi-msg.h"
    +#include "libpoke.h"
    +
    +#define PK_MI_SET_ERRMSG(errmsg, M, ...)  \
    +   sprintf (*errmsg, "[ERROR] " M "\n",\
    +        ##__VA_ARGS__)
    +
    +#define PK_MI_CHECK(errmsg, A, M, ...) \
    +   if (!(A)) {                  \
    +     if (errmsg == NULL) goto error; \
    +     *errmsg = (char *) malloc (1024);    \
    +     PK_MI_SET_ERRMSG(errmsg, M, ##__VA_ARGS__); goto error;}

This is way too complicated.

Let's just have PK_MI_CHECK to get const char*.  Then you don't have to
allocate memory nor to use something like PK_MI_SET_ERRMSG.  If we need
to construct more complicated error messages then we can revisit it.

Also, embed the `if' in the macro body in a do { ... } while (0), so
when you add a trailing semicolon it constitutes a single statement.

    +
    +#define PK_MI_DEBUG(M, ...)                  \
    +      fprintf (stderr, "DEBUG %s:%d: " M "\n",\
    +        __FILE__, __LINE__, ##__VA_ARGS__)
    +
     
     /* Given a string containing a JSON message, parse it and return a MI
        message.
    @@ -40,4 +56,28 @@ const char *pk_mi_msg_to_json (pk_mi_msg msg);
     /* XXX services to pk_val <-> json */

This comment is now obsolete right?

     /* XXX services to pk_type <-> json */
     
    +/* Given a pk val, return the json object associated with this val

Given a Poke value, return...

    +
    +   VAL is the pk_val to be converted.
    +
    +   ERRMSG is a buffer to be allocated that contains any error messages.

As mentioned above, I would prefer ERRMSG to be a constant string.

    +
    +   If ERRMSG is NULL, nothing happens on the buffer.
    +
    +   In case of error return NULL.  */
    +
    +const char *pk_mi_val_to_json (pk_val val, char **errmsg);
    +
    +/* Given a json object, return the pk val associated with it
    +      
    +   STR is the string value of a pk_val to be converted.
    +
    +   ERRMSG is a buffer to be allocated that contains any error messages.
    +
    +   If ERRMSG is NULL, nothing happens on the buffer.  
    +   
    +   In case of error returns PK_NULL.  */
    +
    +pk_val pk_mi_json_to_val (const char *str, char **errmsg);
    +
     #endif /* ! PK_MI_JSON */