[patches] [PATCH 2/4] Move tuple_compare wrappers to key_def.h/.cc

[Vladimir Davydov]

On Tue, Feb 20, 2018 at 12:12:33AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev at gmail.com> [18/02/18 23:27]:
> > > The single-line wrappers don't work without key_def. By putting
> > > them to key_def.h we reduce the number of necessary includes. 
> > 
> > IMHO it's semantically incorrect to put tuple manipulation functions to
> > key_def.h, which is all about key_def helpers. If we ever have to extend
> > tuple_compare() or tuple_extract_key() so that they are not one-line
> > wrappers anymore, we'll have to move them back to tuple_compare.h and
> > tuple_extract_key.h.
> 
> I merely think that the choice of a name should not play a
> significant role: perhaps the name has been chosen incorrectly
> after all, and the functions should be called
> key_def_extract_key(), for example.

Such a name wouldn't be consistent with our own coding conventions - we
tend to name functions as subject_action or object_action_subject. We
would have to name it key_def_extract_key_from_tuple to conform, but IMO
that's a far worse name than tuple_extract_key. Also, that would lead to

  key_def_compare_tuples instead of tuple_compare
  key_def_hash_tuple instead of tuple_hash

> 
> These functions require objects of two different types to produce
> results, struct tuple and struct key_def.
> There are three sets of files using these modules:
> using tuple.h exclusively, using key_def.h exclusively, using both
> types, tuple.h and key_def.h. In practice key_def.h already depends on tuple.h,
> since key_def->compare requires tuple.h.

key_def.h doesn't need to depend on tuple.h - forward declaration would
be enough to declare the virtual methods. OTOH there are source files
that don't need tuple_compare and tuple_extract and only require key_def
manipulation functions, e.g. index_def.c. Including tuple_compare and
tuple_extract_key in them would be redundant.

> 
> So my suggestion is to put the functions closer to the top of 
> the acyclic module dependency graph - in key_def.h and remove
> (or work towards removing) a cyclic dependency between tuple.h and
> key_def.h.