Le 28/03/2017 à 12:22, Jean-Marc Lasgouttes a écrit :
This one is probably for Guillaume.
Coverity complains about missing move constructors for Inset, MathAtom,
Mover, SpecializedMover, FileName, InsetMathNest, InsetMathHull.
The help text says: "This class does not have a user-written move
assignment operator and its copy assignment operator is found to be
applied to rvalue(s), which can be moved to possibly enhance program
performance had the move assignment operator been in place."
At least I would guess that FileName would benefit from it, but what
about the others?
Hi Jean-Marc,
The solution is of course not to make the code more complicated (and
error-prone) with custom move operators. Instead one should rely on
compiler-generated move and copy constructors and assignment operators
in a more systematic manner, sometimes making the implementation simpler
even. See <https://rmf.io/cxx11/rule-of-zero>.
It is good to have in mind that the move operators are not defined
implicitly if there are custom copy operators or a custom destructor.
Inset:
I do not understand why the copy constructor does one thing but the copy
assignment something else. I doubt that users of Inset and its
descendents are all aware of the difference. If not important, I suggest
removing the custom copy constructor. This lets default copy and move
being generated automatically. If important, I suggest marking the
constructor explicit so that it is always called on purpose (then the
others need to be defined as defaulted). See attached patch.
MathAtom:
This is essentially an owning pointer with copy semantics. It can be
simplified by deriving from unique_ptr. See b382b246. To get the most
out of the move operators one should also make sure that MathData can
also be moved. To further audit unnecessary copies you can mark the copy
constructor explicit (as well as that of MathData).
Mover, SpecialisedMover:
This is certainly unimportant. But, they can be added very simply by
declaring them as defaulted (once you do that you have to declare the
copies as defaulted too). See attached.
FileName:
This would be automatically copyable and movable if not for the use of
the pointer to implementation. There is a solution here:
<https://oliora.github.io/2015/12/29/pimpl-and-rule-of-zero.html>. If we
used spimpl.h from there, then all the custom destructor and copies
could be removed, and the moves would be generated automatically. In
fact this could be used to simplify other copyable classes with a
pointer to implementation. Do you think the Boost license allow its
inclusion in src/support?
InsetMathNest and InsetMathHull:
Remove custom copies and destructors using the same ideas as for
MathAtom. To do this one should define a template cached<X> that
consists in X where the copy is overridden to reset the value (assuming
it is very important to do this). Note that defining an empty destructor
explicitly is harmful since it prevents implicit definition of the move
operators.
Of course one should audit and simplify all the classes with these
principles in mind.
Guillaume
>From f69fd0aa1f58284103cb4689b90d02b1648713fa Mon Sep 17 00:00:00 2001
From: Guillaume MM <g...@lyx.org>
Date: Sun, 2 Apr 2017 19:58:44 +0200
Subject: [PATCH 1/2] Specify move constructors
Fix coverity's suggestion to define a move constructor
---
src/insets/Inset.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/insets/Inset.h b/src/insets/Inset.h
index 3d93904..b1494f9 100644
--- a/src/insets/Inset.h
+++ b/src/insets/Inset.h
@@ -589,8 +589,11 @@ public:
protected:
/// Constructors
- Inset(Buffer * buf) : buffer_(buf) {}
- Inset(Inset const &) : buffer_(0) {}
+ explicit Inset(Buffer * buf) : buffer_(buf) {}
+ explicit Inset(Inset const &) : buffer_(0) {}
+ Inset(Inset &&) = default;
+ Inset & operator=(Inset const &) = default;
+ Inset & operator=(Inset &&) = default;
/// replicate ourselves
friend class InsetList;
--
2.7.4
>From 0bce11f97ead15e7cacfaa4ecdcdb2a5a4b69e36 Mon Sep 17 00:00:00 2001
From: Guillaume MM <g...@lyx.org>
Date: Mon, 3 Apr 2017 00:31:37 +0200
Subject: [PATCH 2/2] define move constructors
---
src/Mover.h | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/Mover.h b/src/Mover.h
index 4d1c1c7..3769d1c 100644
--- a/src/Mover.h
+++ b/src/Mover.h
@@ -27,7 +27,12 @@ namespace support { class FileName; }
class Mover
{
public:
+ Mover() = default;
virtual ~Mover() {}
+ Mover(Mover &&) = default;
+ Mover & operator=(Mover &&) = default;
+ Mover(Mover const &) = default;
+ Mover & operator=(Mover const &) = default;
/** Copy file @c from to @c to.
* This version should be used to copy files from the original
@@ -109,9 +114,14 @@ protected:
class SpecialisedMover : public Mover
{
public:
- SpecialisedMover() {}
+ SpecialisedMover() = default;
- virtual ~SpecialisedMover() {}
+ ~SpecialisedMover() {} // override
+
+ SpecialisedMover(SpecialisedMover &&) = default;
+ SpecialisedMover & operator=(SpecialisedMover &&) = default;
+ SpecialisedMover(SpecialisedMover const &) = default;
+ SpecialisedMover & operator=(SpecialisedMover const &) = default;
/** @c command should be of the form
* <code>
--
2.7.4
