Patch Detail
Show a patch.
GET /api/patches/323/?format=api
{ "id": 323, "url": "https://patchwork.libcamera.org/api/patches/323/?format=api", "web_url": "https://patchwork.libcamera.org/patch/323/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20190122134428.10208-1-laurent.pinchart@ideasonboard.com>", "date": "2019-01-22T13:44:28", "name": "[libcamera-devel] cam: options: Don't implement move semantics for OptionsParser::Options", "commit_ref": "a22dcaaa786de67786eeaeb47598ed76339249ad", "pull_url": null, "state": "accepted", "archived": false, "hash": "9c29ab4f05d9b004bb3e4167eb003c57fb4cb1fb", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/323/mbox/", "series": [ { "id": 112, "url": "https://patchwork.libcamera.org/api/series/112/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=112", "date": "2019-01-22T13:44:28", "name": "[libcamera-devel] cam: options: Don't implement move semantics for OptionsParser::Options", "version": 1, "mbox": "https://patchwork.libcamera.org/series/112/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/323/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/323/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F1A060B1B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 22 Jan 2019 14:44:37 +0100 (CET)", "from pendragon.bb.dnainternet.fi\n\t(dfj612yyyyyyyyyyyyyby-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00::2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C778D53E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 22 Jan 2019 14:44:36 +0100 (CET)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548164676;\n\tbh=lFPMRUujrjGbuUcDI3cB+2Gl9zjZKwka2p/ezTTKgvY=;\n\th=From:To:Subject:Date:From;\n\tb=CrnUGImtWbc8yHf63/uQuSuHgvHEEZ1KMckY2HTh4MVR7Bqs6dTWZQYwZnX0FlUIC\n\t/P4P3FX9uMhPfCe9u7a7APYTuW9dwk7FQZ2e/+Y5n5ppma28bUATNrYONPWrhN491B\n\tUVvpHSeXN8GcJHcSU6guuCZWfX3UGU/5rfvxYSAg=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Tue, 22 Jan 2019 15:44:28 +0200", "Message-Id": "<20190122134428.10208-1-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.19.2", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[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": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Tue, 22 Jan 2019 13:44:37 -0000" }, "content": "The compiler creates a move constructor automatically when none is\nsupplied, and it does the right thing by default in this case. Using\nstd::move() inside the function prevents the compiler from doing\nreturn value optimization and actually hinders performances. Using\nstd::move() in the caller is unnecessary, the move constructor is used\nautomatically by the compiler.\n\nFor all these reasons remove the tentative optimization that resulted in\nworse performances and worse code.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n\nIf anyone is interested, here's my test code.\n\n#include <iostream>\n#include <string>\n\nclass Copyable\n{\npublic:\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\nprivate:\n\tstd::string name;\n\tstd::string preserved;\n};\n\nclass CopyableWrapper\n{\npublic:\n\tCopyableWrapper() { }\n\n\tCopyableWrapper(const std::string &name)\n\t\t: value(name)\n\t{\n\t}\n\n\tCopyable value;\n};\n\nCopyable copy(const std::string &name)\n{\n\treturn Copyable(name);\n}\n\nCopyable move(const std::string &name)\n{\n\treturn std::move(Copyable(name));\n}\n\nCopyableWrapper copy_wrap(const std::string &name)\n{\n\treturn CopyableWrapper(name);\n}\n\nint 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\nIt produces the following output.\n\nCopyable::Copyable()\nCopyable::Copyable(\"b\")\nCopyable::Copyable(\"c\")\nCopyable::operator=(const Copyable &b)\nCopyable::operator=(Copyable &&b)\nCopyable::Copyable(\"d\")\nCopyable::Copyable(\"e\")\nCopyable::Copyable(Copyable &&e)\nCopyable::~Copyable(, e)\nCopyable::Copyable(\"f\")\nCopyable::operator=(Copyable &&f)\nCopyable::~Copyable(, f)\nCopyable::Copyable(\"g\")\nCopyable::Copyable(Copyable &&g)\nCopyable::~Copyable(, g)\nCopyable::operator=(Copyable &&g)\nCopyable::~Copyable(, )\nCopyable::Copyable(\"h\")\nCopyable::operator=(Copyable &&h)\nCopyable::~Copyable(, h)\nCopyable::Copyable(\"w\")\nCopyable::Copyable(\"x\")\nCopyable::operator=(Copyable &&x)\nCopyable::~Copyable(, x)\nCopyable::~Copyable(x, w)\nCopyable::~Copyable(e, )\nCopyable::~Copyable(d, d)\nCopyable::~Copyable(h, c)\nCopyable::~Copyable(g, b)\nCopyable::~Copyable(f, default)\n\nAs you can see the \"g\" case it pretty bad, and the \"h\" case isn't better\nthan the \"f\" case. The \"x\" case shows that the move assignment operator\nis used for the inner Copyable in CopyableWrapper automatically, which\nimplies that the compiler has generated a move assignment operator for\nCopyableWrapper.\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(-)", "diff": "diff --git a/src/cam/main.cpp b/src/cam/main.cpp\nindex 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 \ndiff --git a/src/cam/options.cpp b/src/cam/options.cpp\nindex 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();\ndiff --git a/src/cam/options.h b/src/cam/options.h\nindex 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", "prefixes": [ "libcamera-devel" ] }