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

[Niklas Söderlund]

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

Looks good, thanks for sharing your test code.

Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund