[tarantool-patches] Re: [PATCH v2 2/8] tuple: rework updates to improve code extendibility

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [19/09/28 09:11]:
> Hi! Thanks for the review!
> 
> On 03/09/2019 21:20, Konstantin Osipov wrote:
> > * Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [19/09/01 00:33]:
> > 
> > If you create a directory, please make it a standalone library, so
> > that dependencies are controlled, both at compile-time and at
> > link-time.
> 
> I tried - it is not possible. Update depends on tuple_dictionary and
> TUPLE_INDEX_BASE from tuple_format.h. Updates need to be a part of
> the tuple library.
> 
> > I don't see a reason to keep the top-level update api in src/box, 
> > please move it to update/update.h or alike.
> I don't see a reason not to keep it, but I don't mind to move
> all to update/.

Well, if it is not an independent library then it should *not* be
moved into a subdirectory. 

> > Generally I don't (I guess yet?) get why you make the split, I
> > hope to learn in a subsequent patches.
> 
> Because update implementations for each field type are very different
> and independent from each other. And I decided that it would be simpler
> to maintain several relatively small and simple files for each field
> type, than one huge mega-file for all updates. Actually, I borrowed
> that file organization from some other databases, where I've looked
> for an inspiration. Such as mongo.

Extracting update operations for different field types into own
files could actually reduce header file dependencies (e..g. decimal includes are only 
used for decimal field updates), so looks like a good idea, but
you didn't do just that.  You chopped everything into little
pieces, and I am still failing to extract motivation for this.

Is there a rational reason to do it except you felt like it? We
generally don't stick to one-class-per-file paradigm, but
one-module-per-file. What is the module structure that you are
trying to get to with the move? 

> It is debatable. The reasoning above looks fair to me. But we can
> discuss.

Wherever you provided any reasoning, it made some sense, and where
I am not compelled to argue. The questions would not have arisen 
if this reasoning was in code and changeset comments.

-- 
Konstantin Osipov, Moscow, Russia