Re: [coverity again] missing move constructors

[Scott Kostyshak]

On Thu, May 25, 2017 at 01:08:20AM +0200, Guillaume MM wrote:

> Hi Scott, indeed there is no need for this in 2.3 and I meant to reply
> to Jean-Marc later. In addition gcc 4.6 might get in the way and I am
> hoping that 2.4 is the good time to unsupport it. So it's best not to
> have this in 2.3.

Makes sense.

Thanks,

Scott


signature.asc
Description: PGP signature

Re: [coverity again] missing move constructors

[Guillaume MM]

Le 24/05/2017 à 04:59, Scott Kostyshak a écrit :

On Tue, May 09, 2017 at 04:25:15PM +0200, Jean-Marc Lasgouttes wrote:

Le 21/04/2017 à 00:11, Guillaume MM a écrit :

Le 08/04/2017 à 23:05, Jean-Marc Lasgouttes a écrit :



FileName:
This would be automatically copyable and movable if not for the use of
the pointer to implementation.


What is the problem with the pointer?


For motivations see for instance
.


The spimpl template declared there looks good to me. There is no problem
with distributing Boost licenced stuff with LyX, we do it already.

Concerning the patches, thery are in general way above my head, and I trust
your judgment. My remarks are
* it is not nice to have to use unique_ptr allover the place, I
do not really care about implementation details. Is it possible to have the
vector carry InsetLabel objects instead?
* the MouseHover.* files seems to be missing fro your third patch.


Speaking of review, I found that setMouseHover was never used, making
the variable useless. What do you think?


I'm afraid I don't understand what you mean here.


Guillaume, do you feel confident enough to push these patches for 2.3.0?
Or since there is no performance gain, do you think it's best to not
have them in 2.3.0 at this point? It's your call.



Hi Scott, indeed there is no need for this in 2.3 and I meant to reply
to Jean-Marc later. In addition gcc 4.6 might get in the way and I am
hoping that 2.4 is the good time to unsupport it. So it's best not to
have this in 2.3.

Re: [coverity again] missing move constructors

[Scott Kostyshak]

On Tue, May 09, 2017 at 04:25:15PM +0200, Jean-Marc Lasgouttes wrote:
> Le 21/04/2017 à 00:11, Guillaume MM a écrit :
> > Le 08/04/2017 à 23:05, Jean-Marc Lasgouttes a écrit :
> > > 
> > > > FileName:
> > > > This would be automatically copyable and movable if not for the use of
> > > > the pointer to implementation.
> > > 
> > > What is the problem with the pointer?
> > 
> > For motivations see for instance
> > .
> 
> The spimpl template declared there looks good to me. There is no problem
> with distributing Boost licenced stuff with LyX, we do it already.
> 
> Concerning the patches, thery are in general way above my head, and I trust
> your judgment. My remarks are
> * it is not nice to have to use unique_ptr allover the place, I
> do not really care about implementation details. Is it possible to have the
> vector carry InsetLabel objects instead?
> * the MouseHover.* files seems to be missing fro your third patch.
> 
> > Speaking of review, I found that setMouseHover was never used, making
> > the variable useless. What do you think?
> 
> I'm afraid I don't understand what you mean here.

Guillaume, do you feel confident enough to push these patches for 2.3.0?
Or since there is no performance gain, do you think it's best to not
have them in 2.3.0 at this point? It's your call.

Scott


signature.asc
Description: PGP signature

Re: [coverity again] missing move constructors

[Jean-Marc Lasgouttes]

Le 21/04/2017 à 00:11, Guillaume MM a écrit :

Le 08/04/2017 à 23:05, Jean-Marc Lasgouttes a écrit :



FileName:
This would be automatically copyable and movable if not for the use of
the pointer to implementation.


What is the problem with the pointer?


For motivations see for instance
.


The spimpl template declared there looks good to me. There is no problem 
with distributing Boost licenced stuff with LyX, we do it already.


Concerning the patches, thery are in general way above my head, and I 
trust your judgment. My remarks are
* it is not nice to have to use unique_ptr allover the 
place, I do not really care about implementation details. Is it possible 
to have the vector carry InsetLabel objects instead?

* the MouseHover.* files seems to be missing fro your third patch.


Speaking of review, I found that setMouseHover was never used, making
the variable useless. What do you think?


I'm afraid I don't understand what you mean here.

JMarc

Re: [coverity again] missing move constructors

[Guillaume MM]

Le 21/04/2017 à 00:11, Guillaume MM a écrit :


I am thinking about something along the lines of the attached patches.
But to be clear, one should not expect any performance gain. Only some
review, clarification, and simplification of the code.

Speaking of review, I found that setMouseHover was never used, making
the variable useless. What do you think?



Any opinion on the patches?

Guillaume

Re: [coverity again] missing move constructors

[Guillaume MM]

Le 08/04/2017 à 23:05, Jean-Marc Lasgouttes a écrit :


Also, I do not understand this (I know it is the same as the old code)
+explicit Inset(Inset const &) : buffer_(0) {}

What is this good for? The semantics look quite broken to me. Is this
even used? (I guess we should make it private to find out).


Like in InsetMathHull and InsetMathNest, I imagine that this is meant to
implement some sort of cache that one invalidates on copy. It is used in
all the Inset*::clone functions for a start.

operator= on the other hand is used nowhere, so the difference does not 
matter. In the attached I even undefine it.



FileName:
This would be automatically copyable and movable if not for the use of
the pointer to implementation.


What is the problem with the pointer?


For motivations see for instance
.


As long as the implementation does
not contain a backpointer, I would say that copying the pointer (and
setting to null in the original?


This is what I propose to do automatically.


Is this supposed to go through a
destructor at some time?)


Yes, one must be careful about custom destructors with move
operators. There is no magic ensuring that the destructor is not
called after a move.


should be good enough to won the contents.

There is a solution here:

. 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?


I have to take a look.


Thanks.



InsetMathNest and InsetMathHull:
To do this one should define a template cached that
consists in X where the copy is overridden to reset the value (assuming
it is very important to do this).


I really do not know what this thing is.



I am thinking about something along the lines of the attached patches.
But to be clear, one should not expect any performance gain. Only some
review, clarification, and simplification of the code.

Speaking of review, I found that setMouseHover was never used, making
the variable useless. What do you think?

Guillaume
>From 9e0a2bc9ca8b496b5af488634eef905a9f837170 Mon Sep 17 00:00:00 2001
From: Guillaume MM 
Date: Sun, 9 Apr 2017 21:43:33 +0200
Subject: [PATCH 1/3] InsetMathHull: let move operators be defined
 automatically

---
 src/CutAndPaste.cpp  |  11 +++--
 src/mathed/InsetMathHull.cpp | 101 ++-
 src/mathed/InsetMathHull.h   |  19 
 src/mathed/InsetMathNest.cpp |  33 +
 src/mathed/InsetMathNest.h   |  26 +-
 src/mathed/InsetMathRef.cpp  |   4 +-
 src/mathed/MathMacro.cpp |  11 -
 src/mathed/MathMacroTemplate.cpp |   4 +-
 src/support/unique_ptr.h |  29 +++
 9 files changed, 113 insertions(+), 125 deletions(-)

diff --git a/src/CutAndPaste.cpp b/src/CutAndPaste.cpp
index 17dbdc6..e625ebe 100644
--- a/src/CutAndPaste.cpp
+++ b/src/CutAndPaste.cpp
@@ -252,16 +252,17 @@ pasteSelectionHelper(DocIterator const & cur, ParagraphList const & parlist,
 		case MATH_HULL_CODE: {
 			// check for equation labels and resolve duplicates
 			InsetMathHull * ins = it->asInsetMath()->asHullInset();
-			std::vector labels = ins->getLabels();
+			std::vector> const & labels =
+ins->getLabels();
 			for (size_t i = 0; i != labels.size(); ++i) {
 if (!labels[i])
 	continue;
-InsetLabel * lab = labels[i];
-docstring const oldname = lab->getParam("name");
-lab->updateLabel(oldname);
+InsetLabel & lab = *labels[i];
+docstring const oldname = lab.getParam("name");
+lab.updateLabel(oldname);
 // We need to update the buffer reference cache.
 need_update = true;
-docstring const newname = lab->getParam("name");
+docstring const newname = lab.getParam("name");
 if (oldname == newname)
 	continue;
 // adapt the references
diff --git a/src/mathed/InsetMathHull.cpp b/src/mathed/InsetMathHull.cpp
index 38cdd26..d5b9318 100644
--- a/src/mathed/InsetMathHull.cpp
+++ b/src/mathed/InsetMathHull.cpp
@@ -195,12 +195,10 @@ docstring hullName(HullType type)
 	return from_ascii("none");
 }
 
-static InsetLabel * dummy_pointer = 0;
-
 InsetMathHull::InsetMathHull(Buffer * buf)
 	: InsetMathGrid(buf, 1, 1), type_(hullNone), numbered_(1, NUMBER),
-	  numbers_(1, empty_docstring()), label_(1, dummy_pointer),
-	  preview_(new RenderPreview(this))
+	  numbers_(1, empty_docstring()), label_(1),
+	  preview_(make_unique(this))
 {
 	//lyxerr << "sizeof InsetMath: " << sizeof(InsetMath) << endl;
 	//lyxerr << "sizeof MetricsInfo: " << sizeof(MetricsInfo) << endl;
@@ -214,8 +212,8 @@ InsetMathHull::InsetMathHull(Buffer * buf)
 
 InsetMathHull::InsetMathHull(Buffer * buf, HullType type)
 	: InsetMathGrid(buf, getCols(

Re: [coverity again] missing move constructors

[Jean-Marc Lasgouttes]

Le 08/04/2017 à 00:00, Guillaume MM a écrit :

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 .


Thanks for the reference, I printed it out for later.


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.


Note that the _only_ use (according to coverity) of the move assignment 
is InsetMathHull::glueall.


Also, I do not understand this (I know it is the same as the old code)
+   explicit Inset(Inset const &) : buffer_(0) {}

What is this good for? The semantics look quite broken to me. Is this 
even used? (I guess we should make it private to find out).



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).


OK.


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.


This looks good to me. I would say that it should go in.


FileName:
This would be automatically copyable and movable if not for the use of
the pointer to implementation.


What is the problem with the pointer? As long as the implementation does 
not contain a backpointer, I would say that copying the pointer (and 
setting to null in the original? Is this supposed to go through a 
destructor at some time?) should be good enough to won the contents.


There is a solution here:

. 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?


I have to take a look.


InsetMathNest and InsetMathHull:
To do this one should define a template cached that
consists in X where the copy is overridden to reset the value (assuming
it is very important to do this).


I really do not know what this thing is.


Of course one should audit and simplify all the classes with these
principles in mind.


Sure, this is why I did not want to touch anything. Thanks for your input.

JMarc

Re: [coverity again] missing move constructors

[Guillaume MM]

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 .

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:
. 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 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 
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 
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 &