* Re: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage
2020-07-08 12:07 ` [Tarantool-patches] [PATCH] Fix wrong " Aleksandr Lyapunov
@ 2020-07-08 12:15 ` Timur Safin
2020-07-08 12:16 ` Cyrill Gorcunov
2020-07-09 10:24 ` Nikita Pettik
2 siblings, 0 replies; 7+ messages in thread
From: Timur Safin @ 2020-07-08 12:15 UTC (permalink / raw)
To: 'Aleksandr Lyapunov', tarantool-patches
Explicit capture list in lambda - is the correct way, making things clearer!
LGTM
Timur
: -----Original Message-----
: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Behalf Of Aleksandr Lyapunov
: Sent: Wednesday, July 8, 2020 3:07 PM
: To: tarantool-patches@dev.tarantool.org
: Subject: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage
:
: The common pitfall of using a lambda is wrong type of capture -
: by value instead of by reference. The simple example is:
: struct sequence_def *new_def = NULL;
: auto def_guard = make_scoped_guard([=] { free(new_def); });
: // initialize new_def
: The problem is that the lambda captures pointer by value, that
: is NULL and will remain NULL in the lambda while new_def is
: successfully initialized in function scope.
:
: The patch fixes the problem above and a couple of similar mistakes.
: ---
: src/box/alter.cc | 4 ++--
: src/main.cc | 2 +-
: 2 files changed, 3 insertions(+), 3 deletions(-)
:
: diff --git a/src/box/alter.cc b/src/box/alter.cc
: index bb42548..3109fb4 100644
: --- a/src/box/alter.cc
: +++ b/src/box/alter.cc
: @@ -3560,7 +3560,7 @@ on_replace_dd_func(struct trigger * /* trigger */,
: void *event)
: * definition to support upgrade script.
: */
: struct func_def *old_def = NULL, *new_def = NULL;
: - auto guard = make_scoped_guard([=] {
: + auto guard = make_scoped_guard([&old_def, &new_def] {
: free(old_def);
: free(new_def);
: });
: @@ -4431,7 +4431,7 @@ on_replace_dd_sequence(struct trigger * /* trigger
: */, void *event)
: struct tuple *new_tuple = stmt->new_tuple;
:
: struct sequence_def *new_def = NULL;
: - auto def_guard = make_scoped_guard([=] { free(new_def); });
: + auto def_guard = make_scoped_guard([&new_def] { free(new_def); });
:
: struct sequence *seq;
: if (old_tuple == NULL && new_tuple != NULL) { /* INSERT */
: diff --git a/src/main.cc b/src/main.cc
: index 2c96391..e622042 100644
: --- a/src/main.cc
: +++ b/src/main.cc
: @@ -729,7 +729,7 @@ main(int argc, char **argv)
: /* Lua interpeter options, e.g. -e and -l */
: int optc = 0;
: const char **optv = NULL;
: - auto guard = make_scoped_guard([=]{ if (optc) free(optv); });
: + auto guard = make_scoped_guard([&optc, &optv]{ if (optc) free(optv);
: });
:
: static struct option longopts[] = {
: {"help", no_argument, 0, 'h'},
: --
: 2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage
2020-07-08 12:07 ` [Tarantool-patches] [PATCH] Fix wrong " Aleksandr Lyapunov
2020-07-08 12:15 ` Timur Safin
@ 2020-07-08 12:16 ` Cyrill Gorcunov
2020-07-09 10:24 ` Nikita Pettik
2 siblings, 0 replies; 7+ messages in thread
From: Cyrill Gorcunov @ 2020-07-08 12:16 UTC (permalink / raw)
To: Aleksandr Lyapunov; +Cc: tarantool-patches
On Wed, Jul 08, 2020 at 03:07:07PM +0300, Aleksandr Lyapunov wrote:
> The common pitfall of using a lambda is wrong type of capture -
> by value instead of by reference. The simple example is:
> struct sequence_def *new_def = NULL;
> auto def_guard = make_scoped_guard([=] { free(new_def); });
> // initialize new_def
> The problem is that the lambda captures pointer by value, that
> is NULL and will remain NULL in the lambda while new_def is
> successfully initialized in function scope.
>
> The patch fixes the problem above and a couple of similar mistakes.
I think we could use shorter [&] form but this is just
a personal preference. Thank you, good catch!
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage
2020-07-08 12:07 ` [Tarantool-patches] [PATCH] Fix wrong " Aleksandr Lyapunov
2020-07-08 12:15 ` Timur Safin
2020-07-08 12:16 ` Cyrill Gorcunov
@ 2020-07-09 10:24 ` Nikita Pettik
2 siblings, 0 replies; 7+ messages in thread
From: Nikita Pettik @ 2020-07-09 10:24 UTC (permalink / raw)
To: Aleksandr Lyapunov; +Cc: tarantool-patches
On 08 Jul 15:07, Aleksandr Lyapunov wrote:
> The common pitfall of using a lambda is wrong type of capture -
> by value instead of by reference. The simple example is:
> struct sequence_def *new_def = NULL;
> auto def_guard = make_scoped_guard([=] { free(new_def); });
> // initialize new_def
> The problem is that the lambda captures pointer by value, that
> is NULL and will remain NULL in the lambda while new_def is
> successfully initialized in function scope.
>
> The patch fixes the problem above and a couple of similar mistakes.
> ---
Agree with Cyrill that we could simply use &. Anyway LGTM,
let's push this patch.
> src/box/alter.cc | 4 ++--
> src/main.cc | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index bb42548..3109fb4 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3560,7 +3560,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
> * definition to support upgrade script.
> */
> struct func_def *old_def = NULL, *new_def = NULL;
> - auto guard = make_scoped_guard([=] {
> + auto guard = make_scoped_guard([&old_def, &new_def] {
> free(old_def);
> free(new_def);
> });
> @@ -4431,7 +4431,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
> struct tuple *new_tuple = stmt->new_tuple;
>
> struct sequence_def *new_def = NULL;
> - auto def_guard = make_scoped_guard([=] { free(new_def); });
> + auto def_guard = make_scoped_guard([&new_def] { free(new_def); });
>
> struct sequence *seq;
> if (old_tuple == NULL && new_tuple != NULL) { /* INSERT */
> diff --git a/src/main.cc b/src/main.cc
> index 2c96391..e622042 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -729,7 +729,7 @@ main(int argc, char **argv)
> /* Lua interpeter options, e.g. -e and -l */
> int optc = 0;
> const char **optv = NULL;
> - auto guard = make_scoped_guard([=]{ if (optc) free(optv); });
> + auto guard = make_scoped_guard([&optc, &optv]{ if (optc) free(optv); });
>
> static struct option longopts[] = {
> {"help", no_argument, 0, 'h'},
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread