From patchwork Tue Jan 22 13:44:28 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 323 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F1A060B1B for ; Tue, 22 Jan 2019 14:44:37 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (dfj612yyyyyyyyyyyyyby-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00::2]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C778D53E for ; Tue, 22 Jan 2019 14:44:36 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1548164676; bh=lFPMRUujrjGbuUcDI3cB+2Gl9zjZKwka2p/ezTTKgvY=; h=From:To:Subject:Date:From; b=CrnUGImtWbc8yHf63/uQuSuHgvHEEZ1KMckY2HTh4MVR7Bqs6dTWZQYwZnX0FlUIC /P4P3FX9uMhPfCe9u7a7APYTuW9dwk7FQZ2e/+Y5n5ppma28bUATNrYONPWrhN491B UVvpHSeXN8GcJHcSU6guuCZWfX3UGU/5rfvxYSAg= From: Laurent Pinchart 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 Subject: [libcamera-devel] [PATCH] cam: options: Don't implement move semantics for OptionsParser::Options X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Jan 2019 13:44:37 -0000 The compiler creates a move constructor automatically when none is supplied, and it does the right thing by default in this case. Using std::move() inside the function prevents the compiler from doing return value optimization and actually hinders performances. Using std::move() in the caller is unnecessary, the move constructor is used automatically by the compiler. For all these reasons remove the tentative optimization that resulted in worse performances and worse code. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- If anyone is interested, here's my test code. #include #include class Copyable { public: Copyable() : name("default"), preserved("default") { std::cout << "Copyable::Copyable()" << std::endl; } Copyable(const std::string &name) : name(name), preserved(name) { std::cout << "Copyable::Copyable(\"" << name << "\")" << std::endl; } Copyable(const Copyable &other) : name(other.name) { std::cout << "Copyable::Copyable(const Copyable &" << name << ")" << std::endl; } Copyable(Copyable &&other) : name(std::move(other.name)) { std::cout << "Copyable::Copyable(Copyable &&" << name << ")" << std::endl; } ~Copyable() { std::cout << "Copyable::~Copyable(" << name << ", " << preserved << ")" << std::endl; } Copyable &operator=(const Copyable &other) { name = other.name; std::cout << "Copyable::operator=(const Copyable &" << name << ")" << std::endl; return *this; } Copyable &operator=(Copyable &&other) { name = std::move(other.name); std::cout << "Copyable::operator=(Copyable &&" << name << ")" << std::endl; return *this; } private: std::string name; std::string preserved; }; class CopyableWrapper { public: CopyableWrapper() { } CopyableWrapper(const std::string &name) : value(name) { } Copyable value; }; Copyable copy(const std::string &name) { return Copyable(name); } Copyable move(const std::string &name) { return std::move(Copyable(name)); } CopyableWrapper copy_wrap(const std::string &name) { return CopyableWrapper(name); } int main(int, char **) { Copyable a; Copyable b("b"); Copyable c = Copyable("c"); a = b; b = std::move(a); Copyable d = copy("d"); Copyable e = move("e"); a = copy("f"); b = std::move(move("g")); c = std::move(copy("h")); CopyableWrapper w("w"); w = copy_wrap("x"); return 0; } It produces the following output. Copyable::Copyable() Copyable::Copyable("b") Copyable::Copyable("c") Copyable::operator=(const Copyable &b) Copyable::operator=(Copyable &&b) Copyable::Copyable("d") Copyable::Copyable("e") Copyable::Copyable(Copyable &&e) Copyable::~Copyable(, e) Copyable::Copyable("f") Copyable::operator=(Copyable &&f) Copyable::~Copyable(, f) Copyable::Copyable("g") Copyable::Copyable(Copyable &&g) Copyable::~Copyable(, g) Copyable::operator=(Copyable &&g) Copyable::~Copyable(, ) Copyable::Copyable("h") Copyable::operator=(Copyable &&h) Copyable::~Copyable(, h) Copyable::Copyable("w") Copyable::Copyable("x") Copyable::operator=(Copyable &&x) Copyable::~Copyable(, x) Copyable::~Copyable(x, w) Copyable::~Copyable(e, ) Copyable::~Copyable(d, d) Copyable::~Copyable(h, c) Copyable::~Copyable(g, b) Copyable::~Copyable(f, default) As you can see the "g" case it pretty bad, and the "h" case isn't better than the "f" case. The "x" case shows that the move assignment operator is used for the inner Copyable in CopyableWrapper automatically, which implies that the compiler has generated a move assignment operator for CopyableWrapper. src/cam/main.cpp | 2 +- src/cam/options.cpp | 13 +------------ src/cam/options.h | 2 -- 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/src/cam/main.cpp b/src/cam/main.cpp index 22211670c625..0d37039f5349 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -33,7 +33,7 @@ static int parseOptions(int argc, char *argv[]) parser.addOption(OptHelp, "Display this help message", "help"); parser.addOption(OptList, "List all cameras", "list"); - options = std::move(parser.parse(argc, argv)); + options = parser.parse(argc, argv); if (!options.valid()) return -EINVAL; diff --git a/src/cam/options.cpp b/src/cam/options.cpp index d391a0e58436..82acff9bbeea 100644 --- a/src/cam/options.cpp +++ b/src/cam/options.cpp @@ -102,7 +102,7 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv) options.values_[c] = optarg ? optarg : ""; } - return std::move(options); + return options; } void OptionsParser::usage() @@ -160,17 +160,6 @@ OptionsParser::Options::Options() { } -OptionsParser::Options::Options(Options &&other) - : values_(std::move(other.values_)) -{ -} - -OptionsParser::Options &OptionsParser::Options::operator=(Options &&other) -{ - values_ = other.values_; - return *this; -} - bool OptionsParser::Options::valid() const { return !values_.empty(); diff --git a/src/cam/options.h b/src/cam/options.h index 88336dfe3cc6..f99ea7300a71 100644 --- a/src/cam/options.h +++ b/src/cam/options.h @@ -23,8 +23,6 @@ public: class Options { public: Options(); - Options(Options &&other); - Options &operator=(Options &&other); bool valid() const; bool isSet(int opt) const;