Hi!

Thanks for your review.

I guess it is debatable. On the one hand, it will be easier to understand which line did the error happened in, on the other hand, it looks better, when such blocks are combined. "Better layout" might seem minorish, however, if we need to debug this code, we can alway place a breakpoint and look at old_def & new_def intently to discover which one is actually NULL. Also Georgy recommended the opposite thing in his review for patch 5 (https://www.freelists.org/post/tarantool-patches/PATCH-v4-0520-refactoring-clear-privilege-managing-triggers-from-exceptions,1), so i guess we can discuss it verbally.

Sincerely,
Ilya Kosarev

Среда, 30 октября 2019, 13:34 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:

On 23 Sep 18:56, Ilya Kosarev wrote:

Hi!

Just one nit below, LGTM with follow-up.

Sergos


> func_def_new_from_tuple is used in on_replace_dd_func therefore
> it has to be cleared from exceptions. Now it doesn't throw any
> more. It means we also need to clear from exceptions it's
> subsidiary function func_def_get_ids_from_tuple.
> Their usages are updated.
>
> Part of #4247
> ---
> src/box/alter.cc | 149 ++++++++++++++++++++++++++++++-----------------
> 1 file changed, 97 insertions(+), 52 deletions(-)
>
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 10a5f8d64..3d1c59189 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3132,6 +3175,8 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
> });
> old_def = func_def_new_from_tuple(old_tuple);
Shall we return error here, rather than possibly mask it with another one at
the next line?

> new_def = func_def_new_from_tuple(new_tuple);
> + if (old_def == NULL || new_def == NULL)
> + return -1;
> if (func_def_cmp(new_def, old_def) != 0) {
> diag_set(ClientError, ER_UNSUPPORTED, "function",
> "alter");
> --
> 2.17.1
>
>