Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 3/8] json: lexer_eof and token_cmp helper functions

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

On 05/10/2019 15:50, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/09/28 09:11]:
>>>>  /**
>>>> - * Compare JSON token keys.
>>>> + * Compare JSON tokens as nodes of a JSON tree. That is, including
>>>> + * parent references.
>>>>   */
>>>
>>> Please explain how it is useful. Why not json_tree_node_cmp then?
>>
>> What is useful? Being able to compare two JSON nodes? Not sure if I
>> understand the question. I just need to compare two nodes. In several
>> places. And I added a function for that.
> 
> How it is independently useful , given there still exists the old cmp function.
> You added a (yet) another cmp function, the comment should contain
> the usage scenario for it.
> 

Sorry, but it already looks like hypergraphia. This function's body
consists of 12 the simplest possible lines, including braces. Its
purpose is obvious. Just compare two tokens and return something that
is < 0, == 0, or > 0. What a usage scenario can there be except
comparison of two tokens?

You are saying, that all the code should be commented, each function
should be described, even a trivial one, but now I can definitely
say, that it is not necessary, and does not make life much easier. I am
working in a project, that is way bigger than Tarantool, and comments
are written for exceptional cases only, where code is not trivial. And
this does not harm at all. When code is easy to understand, comments
won't help to understand it even better. Moreover, they may confuse.
They become comments just for comments.

Here is the diff:

========================================================================

diff --git a/src/lib/json/json.h b/src/lib/json/json.h
index 08a0ee96c..3218769a1 100644
--- a/src/lib/json/json.h
+++ b/src/lib/json/json.h
@@ -288,7 +288,10 @@ json_token_is_leaf(struct json_token *token)
 
 /**
  * Compare two JSON tokens, not taking into account their tree
- * attributes.
+ * attributes. Only the token values are compared. That might be
+ * used to compare two JSON paths. String comparison of the paths
+ * may not work because the same token can be present in different
+ * forms: ['a'] == .a, for example.
  */
 static inline int
 json_token_cmp(const struct json_token *l, const struct json_token *r)