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 <https://rmf.io/cxx11/rule-of-zero>.
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:
<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?
I have to take a look.
InsetMathNest and InsetMathHull:
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).
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
