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

[Mohammad-Reza Nabipoor]

Hi, Kostas!

Thanks for your code and perseverance :)

I've tried to verify the JSON schema using `jsonschema` tool.
It thinks `pk_sct.json` is invalid, and for `pk_uint.json` says this:

```
3500: 3500 is valid under each of {'$ref': '#/definitions/OffsetValue'}, 
{'$ref': '#/definitions/IntegralValue'}
```

It seems it doesn't like the fact that both of `OffsetValue` and `IntegralValue`
are `{"type": "integer"}`.

So please make the verifier happy :)


And a few comments:


diff --git a/poke/pk-mi-json.c b/poke/pk-mi-json.c
index 297de22b..54089855 100644
--- a/poke/pk-mi-json.c
+++ b/poke/pk-mi-json.c
@@ -32,20 +32,6 @@
 #define J_OK 1
 #define J_NOK 0 /* Not OK */
 
+#define JSON_OBJECT_OBJECT_ADD(obj, key, val, label, errmsg)                  \
+  do {                                                                        \
+    int ret = json_object_object_add (obj, key, val);                         \
+    val = NULL;                                                               \
+    GOTO_IF (ret != 0, label, errmsg, "json_object_object_add (%s) failed",   \
+                                      key);                                   \
+  } while (0)



GNU style please :)
1. Incorrect curly brace after the `do`.
2. `key` alignment.
 

+static int
+pk_type_to_json_array (pk_val p_type_arr, json_object **j_type_arr,
+                       char **errmsg)
+{
...
+
+  tmp = pk_array_type_bound (p_type_arr);
+  if (tmp == PK_NULL)
+      j[bound] = NULL;


De-indent, please :)


+/* Parses JSON object as a Poke type.
  *
  * Returns J_OK on success, J_NOK otherwise.
  */
-static inline int
-pvalue (json_object *obj, pk_val *pval, char **errmsg)
+static int
+json_to_pk_type (json_object *j_type, pk_val *pk_type, char **errmsg)
 {
-  static const struct
-  {
-    int type_code;
-    const char *name;
-  } TYPES[] = {
-    { PK_UNKNOWN, "\"Unknown\"" },
-    { PK_INT, "\"Integer\"" },
-    { PK_UINT, "\"UnsignedInteger\"" },
-    { PK_STRING, "\"String\"" },
-    { PK_OFFSET, "\"Offset\"" },
-    { PK_ARRAY, "\"Array\"" },
-    { PK_STRUCT, "\"Struct\"" },
-    { PK_CLOSURE, "\"Closure\"" },
-    { PK_ANY, "\"Any\"" },
-  };
-  static const int TYPES_LEN = sizeof (TYPES) / sizeof (TYPES[0]);
-  int type_code;
-  const char *type_str;
-  json_object *j_type;
-
-  if (obj == NULL) {
-    *pval = PK_NULL;
-    return J_OK;
-  }
+  const char *code;
+  json_object *j_code;
+  int ret;
+
+#define STR(X) #X
+#define JSON_TO_PK_TYPE(type)                                        \
+  do {                                                               \
+    int ret = json_to_pk_type_##type (j_type, pk_type, errmsg);      \
+    FAIL_IF (ret == J_NOK, errmsg, "invalid " #type " type");        \
+  } while (0)
+
+  ret = jexpect (j_type, "code", json_type_string, &j_code, errmsg);
+  FAIL_IF (ret == J_NOK, errmsg, "expects \"code\" field in \"type\"");
+  code = json_object_get_string (j_code);
+  FAIL_IF (code == NULL, errmsg, "invalid \"code\" field");
+
+  if (STREQ (code, "Integral"))
+    {
+      JSON_TO_PK_TYPE (integral);
+    }
+  else if (STREQ (code, "String"))
+    {
+      *pk_type = pk_make_string_type ();
+    }
+  else if (STREQ (code, "Offset"))
+    {
+      JSON_TO_PK_TYPE (offset);
+    }
+  else if (STREQ (code, "Struct"))
+    {
+      JSON_TO_PK_TYPE (struct);
+    }
+  else if (STREQ (code, "Array"))
+    {
+      JSON_TO_PK_TYPE (array);
+    }
+  else if (STREQ (code, "Any"))
+    {
+      *pk_type = pk_make_any_type ();
+    }
+  else if (STREQ (code, "Void"))
+    {
+      /* TODO: Implement.  */
+      return J_NOK;
+    }
+  else if (STREQ (code, "Closure"))
+    {
+      /* TODO: Implement.  */
+      return J_NOK;
+    }
+  else
+   {
+     FAIL_IF (1, errmsg, "Code has an invalid value");
+   }


You can use the function table approach here, too.
E.g., change this entry
     { PK_INT, "\"Integer\"" },
to
     { PK_INT, "\"Integer\"", pk_json_to_pk_type_integral },

I know that special-cases are annoying. But using tables make the code
uniform.

 
+/* Expects a Poke array value in JSON object.
+ *
+ * Returns J_OK on success, J_NOK otherwise.
+ */
+static int
+json_to_pk_val_array (json_object *j_arr, pk_val pk_arr_type, pk_val *pk_arr,
+                      char **errmsg)
+{
+  pk_val pk_arr_tmp, pk_arr_bound;
+  json_object *j_elems, *j_boffsets, *j_mapping;
+  size_t nelems, size_elems;



We don't need `size_elems`.
Please remove it.



+  int ret, size_p;



Ditto for `size_p`.


 
+  pk_arr_bound = pk_array_type_bound (pk_arr_type);
+  if (pk_arr_bound != PK_NULL)
+    {
+      pk_arr_tmp = pk_make_array (pk_make_uint (nelems, 64), pk_arr_type);
+      switch (pk_type_code (pk_typeof (pk_arr_bound)))
+        {
+          case PK_UINT:
+            size_p = 0;
+            break;
+          case PK_OFFSET:
+            size_p = 1;
+            break;
+          default:
+            assert (0);
+        }
+    }
+  else
+    size_p = -1;



We don't need this chunk, too.


 
-  for (size_t i = 1; i < elems_len; ++i)
+  size_elems = 0;
+  pk_arr_tmp = pk_make_array (pk_make_uint (0, 64), pk_arr_type);
+  for (size_t i = 0 ; i < nelems ; i++)
     {
+      pk_val elem, boffset;
+      json_object *j_elem, *j_boffset;
+      int64_t boffset_val;
+
       j_elem = json_object_array_get_idx (j_elems, i);
-      assert (j_elem != NULL);
-      ARR_JERR (pexpect_aelem (j_elem, &p_value, &p_boffset, errmsg));
-      pk_array_insert_elem (p_arr, i, p_value);
-      RETURN_ERR_IF (
-          !pk_val_equal_p (pk_array_elem_boffset (p_arr, i), p_boffset),
-          errmsg, "invalid Array: boffset mismatch at index %zu", i);
+      ARR_JERR (json_to_pk_val (j_elem, pk_array_type_etype (pk_arr_type),
+                                &elem, errmsg));


You can put `pk_array_type_etype (pk_arr_type)` in a varialbe.


+      pk_array_insert_elem (pk_arr_tmp, i, elem);
+


We don't need the following part:


+      j_boffset = json_object_array_get_idx (j_boffsets, i);
+      ret = json_object_is_type (j_boffset, json_type_int);
+      FAIL_IF (ret == J_NOK, errmsg,
+               "expects JSON item of type \"int\" in \"boffsets\" at index 
%zu",
+               i);
+
+      boffset_val = json_object_get_int64 (j_boffset);
+      FAIL_IF (boffset_val < 0, errmsg,
+               "boffset has non-positive value at index %zu", i);
+      boffset = pk_make_uint ((uint64_t) boffset_val, 64);

+
+      FAIL_IF (!pk_val_equal_p (pk_array_elem_boffset (pk_arr_tmp, i), 
boffset),
+               errmsg, "invalid Array: boffset mismatch at index %zu", i);
+
+      /* Increase by 1 if bound is an integer or by pk_sizeof
+         if bound is a size.  */
+      size_elems += size_p == 1 ? pk_sizeof (elem) : 1;


Up to here.

     }
 
-  ARR_JERR (pexpect_map (j_mapping, p_arr, errmsg));
-  *p_array = p_arr;
+  ARR_JERR (jexpect (j_arr, "mapping", json_type_object, &j_mapping, errmsg));
+
+  ARR_JERR (json_to_pk_val_map (j_mapping, pk_arr_tmp, errmsg));
+  *pk_arr = pk_arr_tmp;
+
   return J_OK;
 
 #undef ARR_JERR
 }
 


diff --git a/testsuite/Makefile.am b/testsuite/Makefile.am
index d0b7ab74..bffe6ab0 100644
--- a/testsuite/Makefile.am
+++ b/testsuite/Makefile.am
@@ -2047,11 +2047,20 @@ EXTRA_DIST = \
   poke.libpoke/pk_nequal_sct.test \
   poke.libpoke/pk_nequal_arr.test \
   poke.mi-json/pk_int.json \
+  poke.mi-json/pk_int_fail.json \
   poke.mi-json/pk_uint.json \
+  poke.mi-json/pk_uint_fail.json \
   poke.mi-json/pk_string.json \
+  poke.mi-json/pk_string_fail.json \
   poke.mi-json/pk_offset.json \
+  poke.mi-json/pk_offset_fail.json \
   poke.mi-json/pk_offset_uint.json \
-  poke.mi-json/pk_array.json \
+  poke.mi-json/pk_offset_uint_fail.json \
   poke.mi-json/pk_sct_empty.json \
   poke.mi-json/pk_sct_empty_mapped.json \
-  poke.mi-json/pk_sct.json
+  poke.mi-json/pk_sct.json \
+  poke.mi-json/pk_sct_fail.json \
+  poke.mi-json/pk_array_bounded.json \
+  poke.mi-json/pk_array_bounded_fail.json \
+  poke.mi-json/pk_array_bounded_offset.json \
+  poke.mi-json/pk_array_bounded_offset_fail.json



I think you forgot to actually add these new files to the patch :)



diff --git a/testsuite/poke.mi-json/pk_string.json 
b/testsuite/poke.mi-json/pk_string.json
index b80b089b..2793a297 100644
--- a/testsuite/poke.mi-json/pk_string.json
+++ b/testsuite/poke.mi-json/pk_string.json
@@ -2,8 +2,8 @@
 "foo"
 ##
 {
-    "PokeValue" : {
-        "type" : "String",
-        "value" : "foo"
-    }
+    "type": {
+        "code": "String",


This extra comma makes JSON invalid. This is correct:
         "code": "String"


+    },
+    "value": "foo"
 }



Regards,
Mohammad-Reza