[{"id":496,"web_url":"https://patchwork.libcamera.org/comment/496/","msgid":"<20190122141351.GC4127@bigcity.dyn.berto.se>","date":"2019-01-22T14:13:51","subject":"Re: [libcamera-devel] [PATCH] cam: options: Don't implement move\n\tsemantics for OptionsParser::Options","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your nice investigation.\n\nOn 2019-01-22 15:44:28 +0200, Laurent Pinchart wrote:\n> The compiler creates a move constructor automatically when none is\n> supplied, and it does the right thing by default in this case. Using\n> std::move() inside the function prevents the compiler from doing\n> return value optimization and actually hinders performances. Using\n> std::move() in the caller is unnecessary, the move constructor is used\n> automatically by the compiler.\n> \n> For all these reasons remove the tentative optimization that resulted in\n> worse performances and worse code.\n> \n> Signed-off-by: Laurent Pinchart \n\nLooks good, thanks for sharing your test code.\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> \n> If anyone is interested, here's my test code.\n> \n> #include \n> #include \n> \n> class Copyable\n> {\n> public:\n> \tCopyable()\n> \t\t: name(\"default\"), preserved(\"default\")\n> \t{\n> \t\tstd::cout << \"Copyable::Copyable()\" << std::endl;\n> \t}\n> \n> \tCopyable(const std::string &name)\n> \t\t: name(name), preserved(name)\n> \t{\n> \t\tstd::cout << \"Copyable::Copyable(\\\"\" << name << \"\\\")\" << std::endl;\n> \t}\n> \n> \tCopyable(const Copyable &other)\n> \t\t: name(other.name)\n> \t{\n> \t\tstd::cout << \"Copyable::Copyable(const Copyable &\" << name << \")\" << std::endl;\n> \t}\n> \n> \tCopyable(Copyable &&other)\n> \t\t: name(std::move(other.name))\n> \t{\n> \t\tstd::cout << \"Copyable::Copyable(Copyable &&\" << name << \")\" << std::endl;\n> \t}\n> \n> \t~Copyable()\n> \t{\n> \t\tstd::cout << \"Copyable::~Copyable(\" << name << \", \" << preserved << \")\" << std::endl;\n> \t}\n> \n> \tCopyable &operator=(const Copyable &other)\n> \t{\n> \t\tname = other.name;\n> \t\tstd::cout << \"Copyable::operator=(const Copyable &\" << name << \")\" << std::endl;\n> \t\treturn *this;\n> \t}\n> \n> \tCopyable &operator=(Copyable &&other)\n> \t{\n> \t\tname = std::move(other.name);\n> \t\tstd::cout << \"Copyable::operator=(Copyable &&\" << name << \")\" << std::endl;\n> \t\treturn *this;\n> \t}\n> \n> private:\n> \tstd::string name;\n> \tstd::string preserved;\n> };\n> \n> class CopyableWrapper\n> {\n> public:\n> \tCopyableWrapper() { }\n> \n> \tCopyableWrapper(const std::string &name)\n> \t\t: value(name)\n> \t{\n> \t}\n> \n> \tCopyable value;\n> };\n> \n> Copyable copy(const std::string &name)\n> {\n> \treturn Copyable(name);\n> }\n> \n> Copyable move(const std::string &name)\n> {\n> \treturn std::move(Copyable(name));\n> }\n> \n> CopyableWrapper copy_wrap(const std::string &name)\n> {\n> \treturn CopyableWrapper(name);\n> }\n> \n> int main(int, char **)\n> {\n> \tCopyable a;\n> \tCopyable b(\"b\");\n> \tCopyable c = Copyable(\"c\");\n> \n> \ta = b;\n> \tb = std::move(a);\n> \n> \tCopyable d = copy(\"d\");\n> \tCopyable e = move(\"e\");\n> \n> \ta = copy(\"f\");\n> \tb = std::move(move(\"g\"));\n> \tc = std::move(copy(\"h\"));\n> \n> \tCopyableWrapper w(\"w\");\n> \n> \tw = copy_wrap(\"x\");\n> \n> \treturn 0;\n> }\n> \n> It produces the following output.\n> \n> Copyable::Copyable()\n> Copyable::Copyable(\"b\")\n> Copyable::Copyable(\"c\")\n> Copyable::operator=(const Copyable &b)\n> Copyable::operator=(Copyable &&b)\n> Copyable::Copyable(\"d\")\n> Copyable::Copyable(\"e\")\n> Copyable::Copyable(Copyable &&e)\n> Copyable::~Copyable(, e)\n> Copyable::Copyable(\"f\")\n> Copyable::operator=(Copyable &&f)\n> Copyable::~Copyable(, f)\n> Copyable::Copyable(\"g\")\n> Copyable::Copyable(Copyable &&g)\n> Copyable::~Copyable(, g)\n> Copyable::operator=(Copyable &&g)\n> Copyable::~Copyable(, )\n> Copyable::Copyable(\"h\")\n> Copyable::operator=(Copyable &&h)\n> Copyable::~Copyable(, h)\n> Copyable::Copyable(\"w\")\n> Copyable::Copyable(\"x\")\n> Copyable::operator=(Copyable &&x)\n> Copyable::~Copyable(, x)\n> Copyable::~Copyable(x, w)\n> Copyable::~Copyable(e, )\n> Copyable::~Copyable(d, d)\n> Copyable::~Copyable(h, c)\n> Copyable::~Copyable(g, b)\n> Copyable::~Copyable(f, default)\n> \n> As you can see the \"g\" case it pretty bad, and the \"h\" case isn't better\n> than the \"f\" case. The \"x\" case shows that the move assignment operator\n> is used for the inner Copyable in CopyableWrapper automatically, which\n> implies that the compiler has generated a move assignment operator for\n> CopyableWrapper.\n> \n> src/cam/main.cpp | 2 +-\n> src/cam/options.cpp | 13 +------------\n> src/cam/options.h | 2 --\n> 3 files changed, 2 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index 22211670c625..0d37039f5349 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -33,7 +33,7 @@ static int parseOptions(int argc, char *argv[])\n> \tparser.addOption(OptHelp, \"Display this help message\", \"help\");\n> \tparser.addOption(OptList, \"List all cameras\", \"list\");\n> \n> -\toptions = std::move(parser.parse(argc, argv));\n> +\toptions = parser.parse(argc, argv);\n> \tif (!options.valid())\n> \t\treturn -EINVAL;\n> \n> diff --git a/src/cam/options.cpp b/src/cam/options.cpp\n> index d391a0e58436..82acff9bbeea 100644\n> --- a/src/cam/options.cpp\n> +++ b/src/cam/options.cpp\n> @@ -102,7 +102,7 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)\n> \t\toptions.values_[c] = optarg ? optarg : \"\";\n> \t}\n> \n> -\treturn std::move(options);\n> +\treturn options;\n> }\n> \n> void OptionsParser::usage()\n> @@ -160,17 +160,6 @@ OptionsParser::Options::Options()\n> {\n> }\n> \n> -OptionsParser::Options::Options(Options &&other)\n> -\t: values_(std::move(other.values_))\n> -{\n> -}\n> -\n> -OptionsParser::Options &OptionsParser::Options::operator=(Options &&other)\n> -{\n> -\tvalues_ = other.values_;\n> -\treturn *this;\n> -}\n> -\n> bool OptionsParser::Options::valid() const\n> {\n> \treturn !values_.empty();\n> diff --git a/src/cam/options.h b/src/cam/options.h\n> index 88336dfe3cc6..f99ea7300a71 100644\n> --- a/src/cam/options.h\n> +++ b/src/cam/options.h\n> @@ -23,8 +23,6 @@ public:\n> \tclass Options {\n> \tpublic:\n> \t\tOptions();\n> -\t\tOptions(Options &&other);\n> -\t\tOptions &operator=(Options &&other);\n> \n> \t\tbool valid() const;\n> \t\tbool isSet(int opt) const;\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2649460B1B\n\tfor ;\n\tTue, 22 Jan 2019 15:13:53 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id l15-v6so20715315lja.9\n\tfor ;\n\tTue, 22 Jan 2019 06:13:53 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tl21-v6sm24664ljj.48.2019.01.22.06.13.51\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 22 Jan 2019 06:13:51 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=MnfdKGibjKXRo/GY9zHIyLSzmHyvhP4VBgtD2LVmwws=;\n\tb=0z0xNfrcnRxB7RCtcRJx3sYURhvcim8CxM0Fuc8CtplCwb7ZtqQ54RPYObKpC+Bxio\n\tO0gLqlfIMTxlHoOWjt+FwYjkZabQ63dY3g2xC2o74IGATjr+dyC5+HQe2sb06gO+Ie3l\n\t/VqI58ZEJSMWJ27LJQ8cAGeNScSmG5OfH+EffSUpN2oV1rBwkkuOQNulPd32j+/mzIk4\n\tA9iZ3Nyw+dCgZZkcYzpp4mZFwRI1HbxgdGB6BP51IDiL7a9X10jqMr/woo91FoT7a7hI\n\tiM0e4qwANr3W3p4L9fmP07CsPwxG2U+3PkR9icAllGZ1e+o5HKtJeWyDF1YW06C8q08h\n\tCjZA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=MnfdKGibjKXRo/GY9zHIyLSzmHyvhP4VBgtD2LVmwws=;\n\tb=HZs8Tt7YuYjc5EjkOZBJ0MuYm2EQI26iM/gxx1Jj7KOMwTK9Tm1b9d6OWatIYujgJD\n\tjHpUqHXloouGzKg7wM42c+WORUAVvItpAxwdFli+kTLd5gLvd45lNUXvW0ZsacUAZ8PN\n\tEEafh/XXbXTkLBXHIqDcFr5EWdOLvYaHcz/NEtuBofiF3kBTZ4qSz3qizFsI16X7Ru5h\n\tFFCXrq982lE8tLSdEoVD7+QDGv0f41XjsQmD8J5Ue9Tdw+KafcswvG8RJ5Ez4HZd8t6W\n\tZu0u3xYtOGWUIc17tFUHpJSnQPV64jULcQd45DA7FDPKwEKnywfEGS02CPf68s8uDlhe\n\tEbEQ==","X-Gm-Message-State":"AJcUukfgXW7o0ShaJuiTgOjrsOeVgDc0qyQnAZnj3O7Kl/db+FTx4oEF\n\tfejZsXQ+P78eqWADSrDupBaHqGzJ72o=","X-Google-Smtp-Source":"ALg8bN5bKKTXiABEl3QtGBVAEXhPncaOTQOKn9gFbzifQn3pZ9xbwphsktl+vHV1MskTQoApB8bHBQ==","X-Received":"by 2002:a2e:7f04:: with SMTP id\n\ta4-v6mr22170687ljd.156.1548166432323; \n\tTue, 22 Jan 2019 06:13:52 -0800 (PST)","Date":"Tue, 22 Jan 2019 15:13:51 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122141351.GC4127@bigcity.dyn.berto.se>","References":"<20190122134428.10208-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190122134428.10208-1-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] cam: options: Don't implement move\n\tsemantics for OptionsParser::Options","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 22 Jan 2019 14:13:53 -0000"}}]