Re: [PATCH] JSON schema refactoring and adaptive code changes.

[Mohammad-Reza Nabipoor]

Hi, Kostas!

It's shocking that I've procrastinated this review for a month! :facepalm:
Time flies :(

Thanks for the version 2 of the patch :)
It's much more readable!


0. The patch still doesn't apply on master (I know it's not a big deal, but
   it'd be nicer).
   I recommend you to take a look at this web page: https://git-rebase.io/
   It's a very good resource on how to edit the git history and make it
   clean before sending a patch. I think it's a very useful skill.
   And if you have any question, feel free to ask on IRC.

1. Here I use a different style of commenting. Instead of inline commenting,
   I use file name and function name to tell you where to look at.

- libpoke/pk-val.c
  Why `#include <stdio.h>`?

- poke/poke-mi-json.c
  - `jfree_all` should return `void`.
    I know that you only call `jfree_call` on failure code paths, but still
    I think it doesn't make any sense for this function to return something.

  - `JSON_OBJECT_OBJECT_ADD` macro:
      - Provide more info in error message, like:
        ```c
        GOTO_IF (ret != 0, label, errmsg, "json_object_object_add (%s) failed",
                 key);
        ```

  - `pk_type_to_json`:
    I think the approach of old `pvalue` is nicer. Instead of a big switch-case,
    the code can be like this:

  ```c
  typedef int (*pk_type_to_json_func) (pk_val, json_object **, char **);

  static const pk_type_to_json_func PK_TYPE_TO_JSON_FUNCTIONS[] = {
    [PK_UNKNOWN] = NULL,
    [PK_INT] = pk_type_to_json_integral,
    [PK_UINT] = pk_type_to_json_integral,
    [PK_STRING] = pk_type_to_json_string,
    [PK_OFFSET] = pk_type_to_json_offset,
    [PK_ARRAY] = pk_type_to_json_array,
    [PK_STRUCT] = pk_type_to_json_struct,
    [PK_CLOSURE] = NULL,
    [PK_ANY] = pk_type_to_json_any,
  };
  #define PK_TYPE_TO_JSON_FUNCTIONS_LEN                                         
\
    (sizeof (PK_TYPE_TO_JSON_FUNCTIONS) / sizeof (PK_TYPE_TO_JSON_FUNCTIONS[0]))

  static int
  pk_type_to_json (pk_val ptype, json_object **j_type, char **errmsg)
  {
    int type_code = pk_type_code (ptype);
    pk_type_to_json_func func;

    assert (type_code < PK_TYPE_TO_JSON_FUNCTIONS_LEN);
    func = PK_TYPE_TO_JSON_FUNCTIONS[type_code];
    FAIL_IF (func == NULL, "invalid Poke type");
    return func (ptype, j_type, errmsg);
  }
  ```

  We can use the same approach for `pk_val_to_json`, too (and also other
  places).

  - `pk_type_to_json_integral`: s/pk_type_int/p_type_int/

  - `pk_type_to_json_string`: s/pk_type_str/p_type_str/

  - `pk_type_to_json_offset`: s/pk_type_off/p_type_off/

  - `pk_type_to_json_struct`: s/pk_type_sct/p_type_sct/

  - `pk_type_to_json_array`:
    - s/pk_type_arr/p_type_arr/

    - An `assert` is more appropriate than a `GOTO_IF` in `default` branch
      of the `switch (pk_type_code (bound_type))` statement.

  - `pk_type_to_json_any`: s/pk_type_any/p_type_any/

  - `pk_type_to_json_null`:
     Change the name to `pk_null_to_json`

     And this function should be called in `pk_mi_val_to_json_1` function,
     like this:
     ```c
     if (val != PK_NULL)
       {
         ret = pk_type_to_json (pk_typeof (val), &j[type], errmsg);
         GOTO_IF (ret == J_NOK, error, errmsg,
                  "failed to create JSON object from Poke type");
         ret = pk_val_to_json (val, &j[value], errmsg);
         GOTO_IF (ret == J_NOK, error, errmsg,
                  "failed to create JSON object from Poke value");
         JSON_OBJECT_OBJECT_ADD (j[obj], "type", j[type], error, errmsg);
         JSON_OBJECT_OBJECT_ADD (j[obj], "value", j[value], error, errmsg);
       }
     else
       {
         ret = pk_null_to_json (&j[type], errmsg);
         GOTO_IF (ret == J_NOK, error, errmsg,
                  "failed to create JSON object for PK_NULL");
         JSON_OBJECT_OBJECT_ADD (j[obj], "type", j[type], error, errmsg);
       }
     ```

   - `pk_val_to_json_map`:
     Put the `if` body in a separate line.
     From
       `if (tmp == PK_NULL) j[ios] = NULL;`
     To
       ```
       if (tmp == PK_NULL)
         j[ios] = NULL;
       ```

  - `pk_val_to_json_map`: You can safely remove the `NOTE(kostas)`, because
    the fix is on master for a while :)
    And de-indent the `j[offset] = NULL;` below the `if`.
    Fix the error messages (remove `expect` in strings).

  - `pk_val_to_json_array`:
    Please don't call `pk_uint_value (nelems)` in the `for` loop.

  - `pk_val_to_json_any`: I'd recommend to return `J_NOK`.

  - `json_to_pk_type`: I'd suggest the same table-driven approach as in
    `pk_type_to_json`.

  - `json_to_pk_val`: Likewise.

  - `json_to_pk_val_array`:
     - Using `assert` is not appropriate here: `assert (pk_arr_bound_value > 
0);`
       Assertions should be used for pre-conditions and post-conditions, not
       for checking user input.

     - As we discussed over Jitsi, we need simplification of logic here.
       So, please get rid of `pk_arr_bound`.
       And just use
         `pk_arr_tmp = pk_make_array (pk_make_uint (0, 64), pk_arr_type);`
       The first argument is just a hint for allocation (0 is totally fine).
       And please remove `size_elems`, `size_p`, and checks for correctness of
       total size of array, and also checks for `boffset`.
       It's complicated and error-prone; also it's not always
       possible to do that (e.g., if the boundary is a closure (in future)).

  - `json_to_pk_val_any`:
    Why are you calling `pk_mi_json_to_val_1`?
    It will call `json_to_pk_val`, which in turn will call `json_to_pk_val_any`,
    again! Am I missed something?

  - `pk_mi_json_object_to_msg`:
    This is incorrect:
      `GOTO_IF (J_NOK, failed, errmsg,"invalid message: expects JSON object");`
    You should use `1` instead of `J_NOK`.
    Or put the `!json_object_is_type (json, json_type_object)` as the first arg.

- testsuite/poke.mi-json/mi-json.c:
  - `pk_fatal`: Please use `exit (EXIT_FAILURE);`.


That's it!
Thanks for your time and sorry for the delay.


Regards,
Mohammad-Reza