[libcamera-devel] [PATCH] cam: options: Don't implement move semantics for OptionsParser::Options

[Laurent Pinchart]

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 <laurent.pinchart at ideasonboard.com>
---

If anyone is interested, here's my test code.

#include <iostream>
#include <string>

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;
-- 
Regards,

Laurent Pinchart