From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v5 05/12] lib: implement JSON tree class for json library References: <20181120164316.vtekyag77waocgco@esperanza> From: Kirill Shcherbatov Message-ID: <7913b2a7-f128-a377-3ec8-3321fe9ad2a8@tarantool.org> Date: Mon, 26 Nov 2018 13:50:18 +0300 MIME-Version: 1.0 In-Reply-To: <20181120164316.vtekyag77waocgco@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov Cc: Kostya Osipov List-ID: > 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))