Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

["Timur Safin" <tsafin@tarantool.org>]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export
: box_key_def_dup
: 
: Thanks for the patch!
: 
: See 2 comments below.
: 
: On 24.09.2020 19:00, Timur Safin wrote:
: > Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`
: 
: 1. Do you really need this method? It looks like it can be done by
: 
: 	old_parts = box_key_def_dump_parts(old_key_def);
: 	new_key_def = box_key_def_new_ex(old_parts);
: 
: So the method seems redundant.

Meh! [I do understand that here we are on a weak grounds of pure taste 
preferences, which usually should be out of discussion during patchset 
review. But if you were asking me...]

Introducing simple accessor method to already existing internal 
function for me, much more preferred way that introducing newer,
extended variance of another, lower level function, and combining 
it with 2nd lower level method. 

Module api, ideally, should be high-level for external module 
developer, thus (holistically speaking) 1 higher-level wrapper 
is better than 2 lower-level wrappers. But, certainly, this is
per personal tastes, thus we could hardly get to the final,
single opinion. 

: 
: > diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
: > index 8dd6eb10b..a419b712c 100644
: > --- a/src/box/key_def_api.h
: > +++ b/src/box/key_def_api.h
: > @@ -173,6 +173,16 @@ box_key_part_def_create(box_key_part_def_t *part);
: >  API_EXPORT box_key_def_t *
: >  box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count);
: >
: > +/**
: > + * Duplicate key_def.
: > + * \param key_def Original key_def.
: > + *
: > + * \retval not NULL Duplicate of src.
: > + * \retval     NULL Memory error.
: 
: 2. OCD mode intensifies.

Take care! (hehe, will fix)

: 
: :(
: Lets use @ everywhere in the new code instead of \.