[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Vladislav Shpilevoy]

Thanks for the patch! See 5 comments below.

On 31/05/2018 14:22, Kirill Shcherbatov wrote:
> Part of #3273.
> ---
>   src/box/box.cc | 10 ++++++++--
>   src/box/box.h  | 14 ++++++++++++++
>   2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 02722d6..9540d85 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -877,7 +877,7 @@ box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple)
>   
>   /* schema_find_id()-like method using only public API */
>   uint32_t
> -box_space_id_by_name(const char *name, uint32_t len)
> +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len)

1. The comment now is wrong - space_id_by_name will be used exactly for
non-public API.

2. space_id_by_name must not take system_space_id as an argument. It must
always use BOX_SPACE_ID.

Do this:

/**
  * ...
  *
  * Return 0 on success, -1 on error.
  * @param[out] space_id BOX_ID_NIL - not found. Else space_id is
  *             saved.
  *
  * ...
  */
static inline int
space_id_by_name_impl(system_space_id, name, len, uint32_t *space_id);

uint32_t
space_id_by_name(name, len)
{
	return space_id_by_name_impl(BOX_SPACE_ID, name, len);
}

uint32_t
box_space_id_by_name(name, len)
{
	return space_id_by_name_impl(BOX_VSPACE_ID, name, len);
}

>   {
>   	if (len > BOX_NAME_MAX)
>   		return BOX_ID_NIL;
> diff --git a/src/box/box.h b/src/box/box.h
> index bdd5d5c..ba33113 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -220,6 +220,20 @@ API_EXPORT int
>   box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple);
>   
>   /**
> + * Find space id by name in specified system space.
> + *
> + * This function performs SELECT request to _vspace system space.

3. Not _vspace.

> + * \param system_space_id space to lookup name.
> + * \param name space name
> + * \param len length of \a name
> + * \retval BOX_ID_NIL on error or if not found (check box_error_last())
> + * \retval space_id otherwise
> + * \sa box_index_id_by_name
> + */
> +uint32_t
> +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len);

4. space_id_by_name is not public function. You must not put it inside
/** \cond public */.

It must be in schema.h like other <something_by_something> functions.

5. Why can not you just port schema_find_id to C?

> +
> +/**
>    * Find space id by name.
>    *
>    * This function performs SELECT request to _vspace system space.
>