From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9D3CC469719 for ; Wed, 7 Oct 2020 14:30:05 +0300 (MSK) Date: Wed, 7 Oct 2020 14:30:21 +0300 From: Alexander Turenko Message-ID: <20201007113021.um52i5zfqi4kal3q@tkn_work_nb> References: <53487cd7-3903-99e1-eb77-aaf24c76f170@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <53487cd7-3903-99e1-eb77-aaf24c76f170@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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