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

Message ID 20190122134428.10208-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit a22dcaaa786de67786eeaeb47598ed76339249ad
Headers show
Series
  • [libcamera-devel] cam: options: Don't implement move semantics for OptionsParser::Options
Related show

Commit Message

Laurent Pinchart Jan. 22, 2019, 1:44 p.m. UTC 
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@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(-)

Comments

Niklas Söderlund Jan. 22, 2019, 2:13 p.m. UTC | #1 
Hi Laurent,

Thanks for your nice investigation.

On 2019-01-22 15:44:28 +0200, Laurent Pinchart wrote:
> 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@ideasonboard.com>

Looks good, thanks for sharing your test code.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
> 
> 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
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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;