[{"id":25429,"web_url":"https://patchwork.libcamera.org/comment/25429/","msgid":"<20221017084610.d3tpmmdk5wye53ab@uno.localdomain>","date":"2022-10-17T08:46:10","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> The ScopeExitActions class is a simple object that performs\n> user-provided actions upon destruction. It is meant to simplify cleanup\n> tasks in error handling paths.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Xavier Roumegue \n> ---\n> Changes since v1:\n>\n> - Rename to ScopeExitActions\n\nI will avoid bikeshedding on names, all the alternatives I can think\nof are not much better\n\n> - Replace defer() with operator+=()\n> - Replace clear() with release()\n> ---\n> include/libcamera/base/utils.h | 13 ++++\n> src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++++++++++++\n> 2 files changed, 145 insertions(+)\n>\n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index eb7bcdf4c173..2941d9c45232 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> \t\treturn a - b;\n> }\n>\n> +class ScopeExitActions\n> +{\n> +public:\n> +\t~ScopeExitActions();\n> +\n> +\tvoid operator+=(std::function &&action);\n> +\tvoid release();\n> +\n> +private:\n> +\tstd::vector> actions_;\n> +};\n> +\n> } /* namespace utils */\n>\n> #ifndef __DOXYGEN__\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index 6a307940448e..eb91da297c57 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)\n> * \\a b\n> */\n>\n> +/**\n> + * \\class ScopeExitActions\n> + * \\brief An object that performs actions upon destruction\n> + *\n> + * The ScopeExitActions class is a simple object that performs user-provided\n> + * actions upon destruction. It is meant to simplify cleanup tasks in error\n> + * handling paths.\n> + *\n> + * When the code flow performs multiple sequential actions that each need a\n> + * corresponding cleanup action, error handling quickly become tedious:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tret = startProducer();\n> + * \tif (ret) {\n> + * \t\tfreeMemory();\n> + * \t\treturn ret;\n> + * \t}\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret) {\n> + * \t\tstopProducer();\n> + * \t\tfreeMemory();\n> + * \t\treturn ret;\n> + * \t}\n> + *\n> + * \treturn 0;\n> + * }\n> + * \\endcode\n> + *\n> + * This is prone to programming mistakes, as cleanup actions can easily be\n> + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> + * to use goto statements:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tret = startProducer();\n> + * \tif (ret)\n> + * \t\tgoto error_free;\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret)\n> + * \t\tgoto error_stop;\n> + *\n> + * \treturn 0;\n> + *\n> + * error_stop:\n> + * \tstopProducer();\n> + * error_free:\n> + * \tfreeMemory();\n> + * \treturn ret;\n> + * }\n> + * \\endcode\n> + *\n> + * While this may be considered better, this solution is still quite\n> + * error-prone. Beside the risk of picking the wrong error label, the error\n> + * handling logic is separated from the normal code flow, which increases the\n> + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> + * goto statements to jump over local variable declarations, which can make\n> + * usage of this pattern more difficult.\n> + *\n> + * The ScopeExitActions class solves these issues by allowing code that\n> + * requires cleanup actions to be grouped with its corresponding deferred error\n> + * handling code:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tScopeExitActions actions;\n> + *\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tactions += [&]() { freeMemory(); };\n> + *\n> + * \tret = startProducer();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tactions += [&]() { stopProducer(); };\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tactions.release();\n> + * \treturn 0;\n> + * }\n> + * \\endcode\n> + *\n> + * Deferred error handlers are executed when the ScopeExitActions instance is\n> + * destroyed, in the reverse order of their addition.\n> + */\n> +\n> +ScopeExitActions::~ScopeExitActions()\n\nDoesn't doxygen complain for the missing documentation ?\n\n> +{\n> +\tfor (const auto &action : utils::reverse(actions_))\n> +\t\taction();\n> +}\n> +\n> +/**\n> + * \\brief Add a deferred action\n> + * \\param[in] action The action\n> + *\n> + * Add a deferred action to the ScopeExitActions. Actions will be called upon\n> + * destruction in the reverse order of their addition.\n> + */\n> +void ScopeExitActions::operator+=(std::function &&action)\n> +{\n> +\tactions_.push_back(std::move(action));\n\nIn case of:\n\n if (error) {\n actions += [&]() { do_something(); };\n return ret;\n }\n\nThe lambda expression is wrapped into a temprary std::function, which\nis moved into the vector. At \"return ret;\" the ScopeExitAction is\ndestroyed and the cleanup actions called in reverse sequence.\n\nIn case of the following, instead:\n\n error = do_first();\n if (error)\n actions += [&]() { clean_first(); };\n\n error = do_second();\n if (error)\n actions += [&]() { clean_second(); };\n\n return error;\n\nThe lambda functions wrapped by the temporaries std::function<> will\nstill be valid or do they live in the if (error) { } scope and once we invoke\nthe action it will point to invalid memory ?\n\nWhat if we instead copy the std::function<> in the vector of\nactions to allow such a pattern ? Would only the wrapper be copied or\nthe target as well ?\n\n> +}\n> +\n> +/**\n> + * \\brief Remove all deferred actions\n\nYou seem to have removed defer from the code but kept in\ndocumentation.\n\nShould the usages of \"deferred actions\" be replaced by \"exit actions\"\nor \"cleanup actions\", or is it intentional ?\n\nThanks\n j\n\n> + *\n> + * This function should be called in scope exit paths that don't need the\n> + * actions to be executed, such as success return paths from a function when\n> + * the ScopeExitActions is used for error cleanup.\n> + */\n> +void ScopeExitActions::release()\n> +{\n> +\tactions_.clear();\n> +}\n> +\n> } /* namespace utils */\n>\n> #ifndef __DOXYGEN__\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C247AC0DA4\n\tfor ;\n\tMon, 17 Oct 2022 08:46:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71CC8603D1;\n\tMon, 17 Oct 2022 10:46:14 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94BA1603D1\n\tfor ;\n\tMon, 17 Oct 2022 10:46:12 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 20C1B20005;\n\tMon, 17 Oct 2022 08:46:11 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665996374;\n\tbh=dXXe762iUgWB3nBty19qz++4J83gy+EmSVLNygCv9iw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nfna6du0GkwvcQBb64NR3gOu/gye/w/hlFdtV5aeNrWDUCfpbCTc3A/9Pqbcry1Jd\n\teYFY+r9gnjFrHwEavC1hNe6WadGBZAcJXa/13ypQ/1NYRrAZKaEEnd4kb1nlwQihEV\n\tYpBTD0lI9/4PaNFCgRljuzgruo11T+0HGd/WddGtJXEEsvpbKBx3SF/EvfsSoFQ5KY\n\twC7ED0y0tVaEpW56WT+ooFGKDfdQYIddgtSV5pTQVSiRUZHGYmTUdtWQlC98Ofp2tB\n\t1m2urBoT70L/hFsDKfP/Yu5bCJWrQS8b0zxgXCT+XTv6pqO/5jIq3d6HvBFBcMLAcv\n\tW3LCwkl/wghww==","Date":"Mon, 17 Oct 2022 10:46:10 +0200","To":"Laurent Pinchart ","Message-ID":"<20221017084610.d3tpmmdk5wye53ab@uno.localdomain>","References":"<20221016210202.1107-1-laurent.pinchart@ideasonboard.com>\n\t<20221016210202.1107-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221016210202.1107-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25430,"web_url":"https://patchwork.libcamera.org/comment/25430/","msgid":"","date":"2022-10-17T08:58:21","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Oct 17, 2022 at 10:46:10AM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > The ScopeExitActions class is a simple object that performs\n> > user-provided actions upon destruction. It is meant to simplify cleanup\n> > tasks in error handling paths.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Xavier Roumegue \n> > ---\n> > Changes since v1:\n> >\n> > - Rename to ScopeExitActions\n> \n> I will avoid bikeshedding on names, all the alternatives I can think\n> of are not much better\n> \n> > - Replace defer() with operator+=()\n> > - Replace clear() with release()\n> > ---\n> > include/libcamera/base/utils.h | 13 ++++\n> > src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++++++++++++\n> > 2 files changed, 145 insertions(+)\n> >\n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index eb7bcdf4c173..2941d9c45232 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> > \t\treturn a - b;\n> > }\n> >\n> > +class ScopeExitActions\n> > +{\n> > +public:\n> > +\t~ScopeExitActions();\n> > +\n> > +\tvoid operator+=(std::function &&action);\n> > +\tvoid release();\n> > +\n> > +private:\n> > +\tstd::vector> actions_;\n> > +};\n> > +\n> > } /* namespace utils */\n> >\n> > #ifndef __DOXYGEN__\n> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > index 6a307940448e..eb91da297c57 100644\n> > --- a/src/libcamera/base/utils.cpp\n> > +++ b/src/libcamera/base/utils.cpp\n> > @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)\n> > * \\a b\n> > */\n> >\n> > +/**\n> > + * \\class ScopeExitActions\n> > + * \\brief An object that performs actions upon destruction\n> > + *\n> > + * The ScopeExitActions class is a simple object that performs user-provided\n> > + * actions upon destruction. It is meant to simplify cleanup tasks in error\n> > + * handling paths.\n> > + *\n> > + * When the code flow performs multiple sequential actions that each need a\n> > + * corresponding cleanup action, error handling quickly become tedious:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * \tint ret = allocateMemory();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tret = startProducer();\n> > + * \tif (ret) {\n> > + * \t\tfreeMemory();\n> > + * \t\treturn ret;\n> > + * \t}\n> > + *\n> > + * \tret = startConsumer();\n> > + * \tif (ret) {\n> > + * \t\tstopProducer();\n> > + * \t\tfreeMemory();\n> > + * \t\treturn ret;\n> > + * \t}\n> > + *\n> > + * \treturn 0;\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > + * to use goto statements:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * \tint ret = allocateMemory();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tret = startProducer();\n> > + * \tif (ret)\n> > + * \t\tgoto error_free;\n> > + *\n> > + * \tret = startConsumer();\n> > + * \tif (ret)\n> > + * \t\tgoto error_stop;\n> > + *\n> > + * \treturn 0;\n> > + *\n> > + * error_stop:\n> > + * \tstopProducer();\n> > + * error_free:\n> > + * \tfreeMemory();\n> > + * \treturn ret;\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * While this may be considered better, this solution is still quite\n> > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > + * handling logic is separated from the normal code flow, which increases the\n> > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > + * goto statements to jump over local variable declarations, which can make\n> > + * usage of this pattern more difficult.\n> > + *\n> > + * The ScopeExitActions class solves these issues by allowing code that\n> > + * requires cleanup actions to be grouped with its corresponding deferred error\n> > + * handling code:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * \tScopeExitActions actions;\n> > + *\n> > + * \tint ret = allocateMemory();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tactions += [&]() { freeMemory(); };\n> > + *\n> > + * \tret = startProducer();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tactions += [&]() { stopProducer(); };\n> > + *\n> > + * \tret = startConsumer();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tactions.release();\n> > + * \treturn 0;\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * Deferred error handlers are executed when the ScopeExitActions instance is\n> > + * destroyed, in the reverse order of their addition.\n> > + */\n> > +\n> > +ScopeExitActions::~ScopeExitActions()\n> \n> Doesn't doxygen complain for the missing documentation ?\n\nNot for destructors, no.\n\n> > +{\n> > +\tfor (const auto &action : utils::reverse(actions_))\n> > +\t\taction();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add a deferred action\n> > + * \\param[in] action The action\n> > + *\n> > + * Add a deferred action to the ScopeExitActions. Actions will be called upon\n> > + * destruction in the reverse order of their addition.\n> > + */\n> > +void ScopeExitActions::operator+=(std::function &&action)\n> > +{\n> > +\tactions_.push_back(std::move(action));\n> \n> In case of:\n> \n> if (error) {\n> actions += [&]() { do_something(); };\n> return ret;\n> }\n> \n> The lambda expression is wrapped into a temprary std::function, which\n> is moved into the vector. At \"return ret;\" the ScopeExitAction is\n> destroyed and the cleanup actions called in reverse sequence.\n> \n> In case of the following, instead:\n> \n> error = do_first();\n> if (error)\n> actions += [&]() { clean_first(); };\n> \n> error = do_second();\n> if (error)\n> actions += [&]() { clean_second(); };\n> \n> return error;\n\nNote this is not the expected usage pattern. Callers will do\n\n\terror = do_first();\n\tif (error)\n\t\treturn error;\n\tactions += [&]() { clean_first(); };\n\n\terror = do_second();\n\tif (error)\n\t\treturn error;\n\tactions += [&]() { clean_second(); };\n\n\tactions.release();\n\treturn 0;\n\n> The lambda functions wrapped by the temporaries std::function<> will\n> still be valid or do they live in the if (error) { } scope and once we invoke\n> the action it will point to invalid memory ?\n\nhttps://en.cppreference.com/w/cpp/language/lambda\n\nClosureType has copy and move constructors and assignment operators.\n\n> What if we instead copy the std::function<> in the vector of\n> actions to allow such a pattern ? Would only the wrapper be copied or\n> the target as well ?\n\nI'm not sure to see what difference it would make.\n\n> > +}\n> > +\n> > +/**\n> > + * \\brief Remove all deferred actions\n> \n> You seem to have removed defer from the code but kept in\n> documentation.\n> \n> Should the usages of \"deferred actions\" be replaced by \"exit actions\"\n> or \"cleanup actions\", or is it intentional ?\n\nI'll fix the leftovers.\n\n> > + *\n> > + * This function should be called in scope exit paths that don't need the\n> > + * actions to be executed, such as success return paths from a function when\n> > + * the ScopeExitActions is used for error cleanup.\n> > + */\n> > +void ScopeExitActions::release()\n> > +{\n> > +\tactions_.clear();\n> > +}\n> > +\n> > } /* namespace utils */\n> >\n> > #ifndef __DOXYGEN__","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A23FCBD16B\n\tfor ;\n\tMon, 17 Oct 2022 08:58:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F004762DF0;\n\tMon, 17 Oct 2022 10:58:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BDA0603D1\n\tfor ;\n\tMon, 17 Oct 2022 10:58:46 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE84FCCF;\n\tMon, 17 Oct 2022 10:58:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665997128;\n\tbh=/2LxNVYY7xpbkuqmWulQj9iOdN5fGl6ODRmjQX1gMFE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zZmE9K7CKr6cYrmXLQoYml5uLY4R6glW+TwTRL/sxiBlrBQ1pTixVCGB0Vtcxj56G\n\txeVOJJeRSZHxspbdz8DFy1asM1f2OhDP3+qfBTUXTVGs1awNSpP1n860hGI1GZ0MLB\n\tW1T6LDNAhZVYXObyY7cYw3YEgeP3KLR/V13MOl/TdKLYQIBmxET6m3x814FSxVGm8t\n\t8nSXm9JZpbbMimCbJZb20VbChXxAUF/OMr0OdNPvzt7bQbPuYujwx30h48RcHEwVZT\n\tDu37tseexYGVN/Qdk3/E7Jh6XjSH5OezjQlQRRFRDEcZp/3ULPRcCExikz+jcAY91f\n\tSyW+gWrtoQ5zA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665997126;\n\tbh=/2LxNVYY7xpbkuqmWulQj9iOdN5fGl6ODRmjQX1gMFE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Hsq2ro/L//zo2JCzmdojYMzdhGDRfDBsfD+dghc3YAfFjEImgV8+Ih3PtghmhMzkT\n\t5JnX016gXx5ATs4OqmbaHckTgop/4YibFmyHuviIeiBlKzD82t7aVyjSfFsD4N+2sV\n\t1qLRK4bxtERsrSgkaeMvBJYHzuGF5wLzVjWtAg+s="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Hsq2ro/L\"; dkim-atps=neutral","Date":"Mon, 17 Oct 2022 11:58:21 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221016210202.1107-1-laurent.pinchart@ideasonboard.com>\n\t<20221016210202.1107-2-laurent.pinchart@ideasonboard.com>\n\t<20221017084610.d3tpmmdk5wye53ab@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221017084610.d3tpmmdk5wye53ab@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25433,"web_url":"https://patchwork.libcamera.org/comment/25433/","msgid":"<20221017092851.vww3q4aokngiecya@uno.localdomain>","date":"2022-10-17T09:28:51","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Oct 17, 2022 at 11:58:21AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Oct 17, 2022 at 10:46:10AM +0200, Jacopo Mondi wrote:\n> > On Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > The ScopeExitActions class is a simple object that performs\n> > > user-provided actions upon destruction. It is meant to simplify cleanup\n> > > tasks in error handling paths.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > Reviewed-by: Xavier Roumegue \n> > > ---\n> > > Changes since v1:\n> > >\n> > > - Rename to ScopeExitActions\n> >\n> > I will avoid bikeshedding on names, all the alternatives I can think\n> > of are not much better\n> >\n> > > - Replace defer() with operator+=()\n> > > - Replace clear() with release()\n> > > ---\n> > > include/libcamera/base/utils.h | 13 ++++\n> > > src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++++++++++++\n> > > 2 files changed, 145 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > > index eb7bcdf4c173..2941d9c45232 100644\n> > > --- a/include/libcamera/base/utils.h\n> > > +++ b/include/libcamera/base/utils.h\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> > > \t\treturn a - b;\n> > > }\n> > >\n> > > +class ScopeExitActions\n> > > +{\n> > > +public:\n> > > +\t~ScopeExitActions();\n> > > +\n> > > +\tvoid operator+=(std::function &&action);\n> > > +\tvoid release();\n> > > +\n> > > +private:\n> > > +\tstd::vector> actions_;\n> > > +};\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > #ifndef __DOXYGEN__\n> > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > > index 6a307940448e..eb91da297c57 100644\n> > > --- a/src/libcamera/base/utils.cpp\n> > > +++ b/src/libcamera/base/utils.cpp\n> > > @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)\n> > > * \\a b\n> > > */\n> > >\n> > > +/**\n> > > + * \\class ScopeExitActions\n> > > + * \\brief An object that performs actions upon destruction\n> > > + *\n> > > + * The ScopeExitActions class is a simple object that performs user-provided\n> > > + * actions upon destruction. It is meant to simplify cleanup tasks in error\n> > > + * handling paths.\n> > > + *\n> > > + * When the code flow performs multiple sequential actions that each need a\n> > > + * corresponding cleanup action, error handling quickly become tedious:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * \tint ret = allocateMemory();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tret = startProducer();\n> > > + * \tif (ret) {\n> > > + * \t\tfreeMemory();\n> > > + * \t\treturn ret;\n> > > + * \t}\n> > > + *\n> > > + * \tret = startConsumer();\n> > > + * \tif (ret) {\n> > > + * \t\tstopProducer();\n> > > + * \t\tfreeMemory();\n> > > + * \t\treturn ret;\n> > > + * \t}\n> > > + *\n> > > + * \treturn 0;\n> > > + * }\n> > > + * \\endcode\n> > > + *\n> > > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > > + * to use goto statements:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * \tint ret = allocateMemory();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tret = startProducer();\n> > > + * \tif (ret)\n> > > + * \t\tgoto error_free;\n> > > + *\n> > > + * \tret = startConsumer();\n> > > + * \tif (ret)\n> > > + * \t\tgoto error_stop;\n> > > + *\n> > > + * \treturn 0;\n> > > + *\n> > > + * error_stop:\n> > > + * \tstopProducer();\n> > > + * error_free:\n> > > + * \tfreeMemory();\n> > > + * \treturn ret;\n> > > + * }\n> > > + * \\endcode\n> > > + *\n> > > + * While this may be considered better, this solution is still quite\n> > > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > > + * handling logic is separated from the normal code flow, which increases the\n> > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > > + * goto statements to jump over local variable declarations, which can make\n> > > + * usage of this pattern more difficult.\n> > > + *\n> > > + * The ScopeExitActions class solves these issues by allowing code that\n> > > + * requires cleanup actions to be grouped with its corresponding deferred error\n> > > + * handling code:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * \tScopeExitActions actions;\n> > > + *\n> > > + * \tint ret = allocateMemory();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tactions += [&]() { freeMemory(); };\n> > > + *\n> > > + * \tret = startProducer();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tactions += [&]() { stopProducer(); };\n> > > + *\n> > > + * \tret = startConsumer();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tactions.release();\n> > > + * \treturn 0;\n> > > + * }\n> > > + * \\endcode\n> > > + *\n> > > + * Deferred error handlers are executed when the ScopeExitActions instance is\n> > > + * destroyed, in the reverse order of their addition.\n> > > + */\n> > > +\n> > > +ScopeExitActions::~ScopeExitActions()\n> >\n> > Doesn't doxygen complain for the missing documentation ?\n>\n> Not for destructors, no.\n>\n> > > +{\n> > > +\tfor (const auto &action : utils::reverse(actions_))\n> > > +\t\taction();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Add a deferred action\n> > > + * \\param[in] action The action\n> > > + *\n> > > + * Add a deferred action to the ScopeExitActions. Actions will be called upon\n> > > + * destruction in the reverse order of their addition.\n> > > + */\n> > > +void ScopeExitActions::operator+=(std::function &&action)\n> > > +{\n> > > +\tactions_.push_back(std::move(action));\n> >\n> > In case of:\n> >\n> > if (error) {\n> > actions += [&]() { do_something(); };\n> > return ret;\n> > }\n> >\n> > The lambda expression is wrapped into a temprary std::function, which\n> > is moved into the vector. At \"return ret;\" the ScopeExitAction is\n> > destroyed and the cleanup actions called in reverse sequence.\n> >\n> > In case of the following, instead:\n> >\n> > error = do_first();\n> > if (error)\n> > actions += [&]() { clean_first(); };\n> >\n> > error = do_second();\n> > if (error)\n> > actions += [&]() { clean_second(); };\n> >\n> > return error;\n>\n> Note this is not the expected usage pattern. Callers will do\n>\n> \terror = do_first();\n> \tif (error)\n> \t\treturn error;\n> \tactions += [&]() { clean_first(); };\n>\n> \terror = do_second();\n> \tif (error)\n> \t\treturn error;\n> \tactions += [&]() { clean_second(); };\n>\n> \tactions.release();\n> \treturn 0;\n\nWhy ? What prevents the usage of ScopeExitAction in patterns where\ninstead of returning immediately, you continue ?\n\nIf we introduce a contruct and only prescribe a single usage pattern,\nwe would have to review it even more carefully.\n\n>\n> > The lambda functions wrapped by the temporaries std::function<> will\n> > still be valid or do they live in the if (error) { } scope and once we invoke\n> > the action it will point to invalid memory ?\n>\n> https://en.cppreference.com/w/cpp/language/lambda\n>\n> ClosureType has copy and move constructors and assignment operators.\n>\n> > What if we instead copy the std::function<> in the vector of\n> > actions to allow such a pattern ? Would only the wrapper be copied or\n> > the target as well ?\n>\n> I'm not sure to see what difference it would make.\n>\n\nI was wondering if moving the std::function<> moves the target as\nwell, or it will end up pointing to invalid memory.\n\nIn other words, is the above usage pattern valid or will it crash ?\n\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Remove all deferred actions\n> >\n> > You seem to have removed defer from the code but kept in\n> > documentation.\n> >\n> > Should the usages of \"deferred actions\" be replaced by \"exit actions\"\n> > or \"cleanup actions\", or is it intentional ?\n>\n> I'll fix the leftovers.\n>\n> > > + *\n> > > + * This function should be called in scope exit paths that don't need the\n> > > + * actions to be executed, such as success return paths from a function when\n> > > + * the ScopeExitActions is used for error cleanup.\n> > > + */\n> > > +void ScopeExitActions::release()\n> > > +{\n> > > +\tactions_.clear();\n> > > +}\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > #ifndef __DOXYGEN__\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 75E54BD16B\n\tfor ;\n\tMon, 17 Oct 2022 09:28:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4A9E62DF1;\n\tMon, 17 Oct 2022 11:28:54 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 920B7603D1\n\tfor ;\n\tMon, 17 Oct 2022 11:28:53 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 1AAB624000E;\n\tMon, 17 Oct 2022 09:28:52 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665998934;\n\tbh=gDZXKbeqxfyMLQobWSxQZjSmbWHmpPHXJAm/DBWRqN0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=VCQ0fxMq858ZbJzopEtfmDmkrjJ1bg14JvngGR18UHgc+0T9AIJEJ2HKk3BNMHQB2\n\tiPWpz7W6bmXC67gxzk7CZDrjqli5Y4byJmrCwzkRCsrnHnqhwmQW1gGHt/SgD+7rrP\n\tOzcgy0wtbsN7EzbNNX3MH1ZVAoocJBWhep3uW/JkIeUPLvF6gXAaG6vNsaqO3i+Cl8\n\tVWmxBUlhEzLFAf1nVd0ETl08yf+uW97HBcMz4UcXkYbGhuBS43l2RFt0cz7bF0oEJF\n\ttv2PsQzoEPbPXO82bd/VtaugAM0Yh9isvWu0AeaYPSN1fhBQvK/khreQTSzpTaRW0X\n\tQMwHZXLlfKkLg==","Date":"Mon, 17 Oct 2022 11:28:51 +0200","To":"Laurent Pinchart ","Message-ID":"<20221017092851.vww3q4aokngiecya@uno.localdomain>","References":"<20221016210202.1107-1-laurent.pinchart@ideasonboard.com>\n\t<20221016210202.1107-2-laurent.pinchart@ideasonboard.com>\n\t<20221017084610.d3tpmmdk5wye53ab@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25436,"web_url":"https://patchwork.libcamera.org/comment/25436/","msgid":"","date":"2022-10-17T09:50:08","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Oct 17, 2022 at 11:28:51AM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 17, 2022 at 11:58:21AM +0300, Laurent Pinchart wrote:\n> > On Mon, Oct 17, 2022 at 10:46:10AM +0200, Jacopo Mondi wrote:\n> > > On Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > The ScopeExitActions class is a simple object that performs\n> > > > user-provided actions upon destruction. It is meant to simplify cleanup\n> > > > tasks in error handling paths.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > Reviewed-by: Xavier Roumegue \n> > > > ---\n> > > > Changes since v1:\n> > > >\n> > > > - Rename to ScopeExitActions\n> > >\n> > > I will avoid bikeshedding on names, all the alternatives I can think\n> > > of are not much better\n> > >\n> > > > - Replace defer() with operator+=()\n> > > > - Replace clear() with release()\n> > > > ---\n> > > > include/libcamera/base/utils.h | 13 ++++\n> > > > src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++++++++++++\n> > > > 2 files changed, 145 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > > > index eb7bcdf4c173..2941d9c45232 100644\n> > > > --- a/include/libcamera/base/utils.h\n> > > > +++ b/include/libcamera/base/utils.h\n> > > > @@ -9,6 +9,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> > > > \t\treturn a - b;\n> > > > }\n> > > >\n> > > > +class ScopeExitActions\n> > > > +{\n> > > > +public:\n> > > > +\t~ScopeExitActions();\n> > > > +\n> > > > +\tvoid operator+=(std::function &&action);\n> > > > +\tvoid release();\n> > > > +\n> > > > +private:\n> > > > +\tstd::vector> actions_;\n> > > > +};\n> > > > +\n> > > > } /* namespace utils */\n> > > >\n> > > > #ifndef __DOXYGEN__\n> > > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > > > index 6a307940448e..eb91da297c57 100644\n> > > > --- a/src/libcamera/base/utils.cpp\n> > > > +++ b/src/libcamera/base/utils.cpp\n> > > > @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)\n> > > > * \\a b\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\class ScopeExitActions\n> > > > + * \\brief An object that performs actions upon destruction\n> > > > + *\n> > > > + * The ScopeExitActions class is a simple object that performs user-provided\n> > > > + * actions upon destruction. It is meant to simplify cleanup tasks in error\n> > > > + * handling paths.\n> > > > + *\n> > > > + * When the code flow performs multiple sequential actions that each need a\n> > > > + * corresponding cleanup action, error handling quickly become tedious:\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + * {\n> > > > + * \tint ret = allocateMemory();\n> > > > + * \tif (ret)\n> > > > + * \t\treturn ret;\n> > > > + *\n> > > > + * \tret = startProducer();\n> > > > + * \tif (ret) {\n> > > > + * \t\tfreeMemory();\n> > > > + * \t\treturn ret;\n> > > > + * \t}\n> > > > + *\n> > > > + * \tret = startConsumer();\n> > > > + * \tif (ret) {\n> > > > + * \t\tstopProducer();\n> > > > + * \t\tfreeMemory();\n> > > > + * \t\treturn ret;\n> > > > + * \t}\n> > > > + *\n> > > > + * \treturn 0;\n> > > > + * }\n> > > > + * \\endcode\n> > > > + *\n> > > > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > > > + * to use goto statements:\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + * {\n> > > > + * \tint ret = allocateMemory();\n> > > > + * \tif (ret)\n> > > > + * \t\treturn ret;\n> > > > + *\n> > > > + * \tret = startProducer();\n> > > > + * \tif (ret)\n> > > > + * \t\tgoto error_free;\n> > > > + *\n> > > > + * \tret = startConsumer();\n> > > > + * \tif (ret)\n> > > > + * \t\tgoto error_stop;\n> > > > + *\n> > > > + * \treturn 0;\n> > > > + *\n> > > > + * error_stop:\n> > > > + * \tstopProducer();\n> > > > + * error_free:\n> > > > + * \tfreeMemory();\n> > > > + * \treturn ret;\n> > > > + * }\n> > > > + * \\endcode\n> > > > + *\n> > > > + * While this may be considered better, this solution is still quite\n> > > > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > > > + * handling logic is separated from the normal code flow, which increases the\n> > > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > > > + * goto statements to jump over local variable declarations, which can make\n> > > > + * usage of this pattern more difficult.\n> > > > + *\n> > > > + * The ScopeExitActions class solves these issues by allowing code that\n> > > > + * requires cleanup actions to be grouped with its corresponding deferred error\n> > > > + * handling code:\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + * {\n> > > > + * \tScopeExitActions actions;\n> > > > + *\n> > > > + * \tint ret = allocateMemory();\n> > > > + * \tif (ret)\n> > > > + * \t\treturn ret;\n> > > > + *\n> > > > + * \tactions += [&]() { freeMemory(); };\n> > > > + *\n> > > > + * \tret = startProducer();\n> > > > + * \tif (ret)\n> > > > + * \t\treturn ret;\n> > > > + *\n> > > > + * \tactions += [&]() { stopProducer(); };\n> > > > + *\n> > > > + * \tret = startConsumer();\n> > > > + * \tif (ret)\n> > > > + * \t\treturn ret;\n> > > > + *\n> > > > + * \tactions.release();\n> > > > + * \treturn 0;\n> > > > + * }\n> > > > + * \\endcode\n> > > > + *\n> > > > + * Deferred error handlers are executed when the ScopeExitActions instance is\n> > > > + * destroyed, in the reverse order of their addition.\n> > > > + */\n> > > > +\n> > > > +ScopeExitActions::~ScopeExitActions()\n> > >\n> > > Doesn't doxygen complain for the missing documentation ?\n> >\n> > Not for destructors, no.\n> >\n> > > > +{\n> > > > +\tfor (const auto &action : utils::reverse(actions_))\n> > > > +\t\taction();\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Add a deferred action\n> > > > + * \\param[in] action The action\n> > > > + *\n> > > > + * Add a deferred action to the ScopeExitActions. Actions will be called upon\n> > > > + * destruction in the reverse order of their addition.\n> > > > + */\n> > > > +void ScopeExitActions::operator+=(std::function &&action)\n> > > > +{\n> > > > +\tactions_.push_back(std::move(action));\n> > >\n> > > In case of:\n> > >\n> > > if (error) {\n> > > actions += [&]() { do_something(); };\n> > > return ret;\n> > > }\n> > >\n> > > The lambda expression is wrapped into a temprary std::function, which\n> > > is moved into the vector. At \"return ret;\" the ScopeExitAction is\n> > > destroyed and the cleanup actions called in reverse sequence.\n> > >\n> > > In case of the following, instead:\n> > >\n> > > error = do_first();\n> > > if (error)\n> > > actions += [&]() { clean_first(); };\n> > >\n> > > error = do_second();\n> > > if (error)\n> > > actions += [&]() { clean_second(); };\n> > >\n> > > return error;\n> >\n> > Note this is not the expected usage pattern. Callers will do\n> >\n> > \terror = do_first();\n> > \tif (error)\n> > \t\treturn error;\n> > \tactions += [&]() { clean_first(); };\n> >\n> > \terror = do_second();\n> > \tif (error)\n> > \t\treturn error;\n> > \tactions += [&]() { clean_second(); };\n> >\n> > \tactions.release();\n> > \treturn 0;\n> \n> Why ? What prevents the usage of ScopeExitAction in patterns where\n> instead of returning immediately, you continue ?\n> \n> If we introduce a contruct and only prescribe a single usage pattern,\n> we would have to review it even more carefully.\n\nIt's not that something *prevents* it, I'm just not sure to see what the\ncode above is supposed to do. In any case, calling operator+=() in\nsub-scope is absolutely valid, there's an example in patch 2/2.\n\n> > > The lambda functions wrapped by the temporaries std::function<> will\n> > > still be valid or do they live in the if (error) { } scope and once we invoke\n> > > the action it will point to invalid memory ?\n> >\n> > https://en.cppreference.com/w/cpp/language/lambda\n> >\n> > ClosureType has copy and move constructors and assignment operators.\n> >\n> > > What if we instead copy the std::function<> in the vector of\n> > > actions to allow such a pattern ? Would only the wrapper be copied or\n> > > the target as well ?\n> >\n> > I'm not sure to see what difference it would make.\n> \n> I was wondering if moving the std::function<> moves the target as\n> well, or it will end up pointing to invalid memory.\n> \n> In other words, is the above usage pattern valid or will it crash ?\n\nAs far as I can tell, it will not crash.\n\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Remove all deferred actions\n> > >\n> > > You seem to have removed defer from the code but kept in\n> > > documentation.\n> > >\n> > > Should the usages of \"deferred actions\" be replaced by \"exit actions\"\n> > > or \"cleanup actions\", or is it intentional ?\n> >\n> > I'll fix the leftovers.\n> >\n> > > > + *\n> > > > + * This function should be called in scope exit paths that don't need the\n> > > > + * actions to be executed, such as success return paths from a function when\n> > > > + * the ScopeExitActions is used for error cleanup.\n> > > > + */\n> > > > +void ScopeExitActions::release()\n> > > > +{\n> > > > +\tactions_.clear();\n> > > > +}\n> > > > +\n> > > > } /* namespace utils */\n> > > >\n> > > > #ifndef __DOXYGEN__","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B3A11BD16B\n\tfor ;\n\tMon, 17 Oct 2022 09:50:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BF2F62DF6;\n\tMon, 17 Oct 2022 11:50:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8249B603D1\n\tfor ;\n\tMon, 17 Oct 2022 11:50:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D688ECCF;\n\tMon, 17 Oct 2022 11:50:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666000235;\n\tbh=9PKWwVUk1cqFiP79BnyZd735RPErXCjwhLmjqG1Hz1U=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rZdaa5mvFR6+OWlEO8dg8aHOf0euX0AdRdXOu7ZFrddKkP9FLhmUPdpEZSg+6zF7e\n\tD0Ioj7kGox878GtpbmjbdTG+jR1r41CRo43QxdH7rX92jkohq1a4eUe/mgwZ3tDjbW\n\tdB9QBxKyBSfTwehyHhs7rbi4HPLVQk1skmfSQt+fDsp0R54pewZ89qm8RF1kQenjxW\n\tfeRXlzY15PWbL4QcFEgHgcTKNoMyaIeKQf1ZC4JlRfwh2/Ppo8B4bT2em6pmw6qe5E\n\twqyqqWLxgk/JoXN3WnaX+DajlAinE3rC0aOaj6HaLGNCX0g+5QrW1n5rtdcGHjQXQ0\n\tsb+0lJTdOSqbg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666000233;\n\tbh=9PKWwVUk1cqFiP79BnyZd735RPErXCjwhLmjqG1Hz1U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lJIOCDxhCGqelEf4cGVwJzZtURDPaX5k8anTyQbH00hVOLlymlqoW0op4Avmo7t9C\n\t9i0WTxLGxIhso4VhaJG+12xI2iXXCdulQbZxL1MkyHWmw2tzefM4rrz9KR1nPzC07a\n\tvv9dRtsVfzAWJ+/70zXRWiPyuRnPiJK1lYBlVJsE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lJIOCDxh\"; dkim-atps=neutral","Date":"Mon, 17 Oct 2022 12:50:08 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221016210202.1107-1-laurent.pinchart@ideasonboard.com>\n\t<20221016210202.1107-2-laurent.pinchart@ideasonboard.com>\n\t<20221017084610.d3tpmmdk5wye53ab@uno.localdomain>\n\t\n\t<20221017092851.vww3q4aokngiecya@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221017092851.vww3q4aokngiecya@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add\n\tScopeExitActions class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]