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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 2 comments below.

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.

> 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'.

> +/*
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */