[tarantool-patches] Re: [PATCH v5 05/12] lib: implement JSON tree class for json library

[Kirill Shcherbatov]

> First, I agree with Kostja that naming looks confusing. Please try to
> come up with better names and send them to us for review before
> reworking the patch so that we could agree first.
> 
> Also, once we agree, don't forget to fix comments, function and local
> variable names. For example, if you decide to rename json_path_node to
> json_path_token and json_path_parser to json_path_tokenizer, you should
> also rename all local variables referring to them respectively, e.g.
> node => token, parser => tokenizer and so forth. Please avoid calling
> local variable 'entry' if the struct is called json_path_node or vice
> versa, in other words be consistent.
I've renamed parser class in separate patch:
json_path_parser parser --> json_lexer lexer
json_path_node node --> json_token token

Then, I've moved lexer class to json.{c, h} headers, have made old token
internals as a key-part of new json_token class that is used to organize 
JSON tree.

So we have json_lexer class that emits json_token(s) that may be add(ed)
in json_tree class managing them with mh_json hashtable.
 
> 
> My another concern is comments to the code and function members. They
> look better than they used to be, but still not perfect. E.g. take this
> comment:
I've tried to be more pedantic describing details.

> What is the order of entries in this array for a json array/map?
> 
> Please be more thorough when writing comments. Try to explain things
> that might not be obvious to the reader. There are quite a few typos
> and grammar mistakes, too. I don't want to go over every place in the
> code - please try to do some self-review.
> 
> Plus, see a few comments inline.
Ugum

> unreachable() may turn into nothing in release mode, in which case the
> compiler will probably complain that the return value is missing.
Ok, fixed.

> mh_json_tree_node_new() - but it's not a node, it's a hash table.
> Please fix the name.
mh_json_new()

> May be, it's worth allocating the hash table on demand? It'd simplify
> the API.
> Please don't use 'unlikely' on cold paths - it is meaningless.
Ok.

> Wouldn't it be worth looking up in the children array directly in case
> of JSON_PATH_NUM? BTW, then we wouldn't have to access 'children'
> directly in tuple_format_field() (next patch) thus violating
> encapsulation - instead we could use this helper there as well.
Now works this way.

> We usually double the size with each allocation. If this is intentional,
> please add a comment.
Ok, this required additional field child_size be introduced.
 

> I don't think you really need this function - it's not used anywhere in
> the following patches.
Now json_tree_del is an important part of destructors. Now we have a
consistent API: json_token emitted by lexer doesn't require construction.
Dynamic members constructions are performed on json_tree_add routine.
So pair json_tree_del releases memory respectively

> rlist isn't used anymore
> why do you need inttypes?
> stdint is included twice
> Please cleanup the headers.
Done.

> I really don't like that the caller has to pass rolling_hash explicitly
> as this obfuscates the API. Judging by the following patches, you always
> compute it as json_path_node_hash(path_node, parent->rolling_hash) so
> why not just hide it under the hood? Same goes for json_tree_add() and
> json_tree_remove().
> 
> Also, I'd rather call this function simply json_tree_lookup().
Already done.

> This is barely understandable. Please rewrite without introducing extra
> loop variable, similarly to how rlist_foreach_entry_safe is implemented.
> Also, we use tabs, not spaces for indentation.
Like this:

/** Make pre-order traversal in JSON tree and return entry. */
#define json_tree_foreach_entry_postorder(node, root, type, member)	     \
for ((node) = json_tree_postorder_next_entry(NULL, (root), type, member);    \
     (node) != NULL;							     \
     (node) = json_tree_postorder_next_entry(&(node)->member, (root), type,  \
     					     member))