Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions

[Alexander Turenko <alexander.turenko@tarantool.org>]

> On 23.09.2020 03:14, Alexander Turenko wrote:
> > I plan to expand module API for key_def in further commits and mix of
> > public and private structures and functions becomes hard to read. So it
> > looks meaningful to extract module API wrappers into its own compilation
> > unit.
> 
> 1. I don't think it is a good idea. Not only it is inconsistent with other
> modules such as tuple and fiber, but also it makes not possible to use
> key_def.c static functions, and you need to start moving things to the
> header to be able to use them. In the result, the code becomes harder
> to follow.
> 
> If you want to separate public and non-public methods, you may consider
> grouping them inside the same file. But moving to a new file does not
> look good. Also the new file name looks somewhat 'ugly'. You are
> basically trying to re-invent module.h and the way it is built but
> per-submodule.

Now I remember my primary motivation to splitting key_def and
key_def_api (sorry that I didn't described it in the commit message: I
forgot about it). My wish was to don't depend on the fiber compilation
unit to obtain &fiber()->gc (the box region) in key_def.c.

However the root of the problem is that the box region is exposed from
the fiber compilation unit. So I agree: it should be resolved in some
other way.

As Vlad pointed me, a dependency on the core library is okay for a
compilation unit of the box library in general.

I'll use grouping inside key_def.[ch] the next patchset version.

> 
> > Added libtuple.a to the so called reexport libraries list, because
> > otherwise the functions from key_def_api compilation unit are not
> > exported on 1.10 (in the backported patch).
> > 
> > Part of #5273
> > ---
> > diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> > new file mode 100644
> > index 000000000..5b1c861f5
> > --- /dev/null
> > +++ b/src/box/key_def_api.h
> > @@ -0,0 +1,101 @@
> > +#ifndef TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
> > +#define TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
> 
> 2. Lets use '#pragma once'.

 | Use header guards.

https://www.tarantool.io/en/doc/latest/dev_guide/c_style_guide/

Rules are rules.

You know, I don't like '#pragra once', because it works in non-obvious
way, had various bugs in the past and may slow down compilation ([1]).
So I will follow the accepted style.

(Anyway, I'm going to drop this file and move everything to key_def.h as
you suggested.)

[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58770