[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] JSON schema refactoring and adaptive code changes.
From: |
Mohammad-Reza Nabipoor |
Subject: |
Re: [PATCH] JSON schema refactoring and adaptive code changes. |
Date: |
Wed, 20 Oct 2021 01:31:47 +0330 |
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH] JSON schema refactoring and adaptive code changes.,
Mohammad-Reza Nabipoor <=