[libcamera-devel] [PATCH] cam: options: Don't implement move semantics for OptionsParser::Options
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Jan 22 15:13:51 CET 2019
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
More information about the libcamera-devel
mailing list