Re: [lmi] [lmi-commits] master 6a59da54 8/8: When in doubt, prefer the Rule of Five to the Rule of Zero

[Vadim Zeitlin]

On Mon, 11 Jul 2022 16:22:14 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 7/11/22 12:12, Vadim Zeitlin wrote:
GC> > On Sun, 10 Jul 2022 20:41:31 -0400 (EDT) Greg Chicares 
<gchicares@sbcglobal.net> wrote:
GC> > 
GC> > GC>     In class product_data, the copy ctor and copy assignment operator 
had
GC> > GC>     been private, with the former implemented out of line and the 
latter
GC> > GC>     deleted. The deleted member needn't be private. Perhaps the copy 
ctor
GC> > GC>     should be private, or perhaps not; that will be considered next, 
but
GC> > GC>     for now it's public.
GC> > 
GC> >  I suspect it might have remained private by mistake after the changes of
GC> > c9dd0798c (Add and test product_data copy ctor, 2018-11-03) which didn't
GC> > update its visibility after allowing it to be used. But I'm not completely
GC> > sure neither.
GC> 
GC> Actually, it's used nowhere but in that unit test: if we remove the
GC> copy ctor, everything builds correctly except for that unit test.
GC> Commit c9dd0798c suggests that it was needed at one time, but
GC> apparently it isn't needed any longer.
GC> 
GC> My first thought was to remove the copy ctor and the unit test.

 I agree with this, it doesn't seem necessary to allow the objects of this
class to be copied and, being rather big, copying them is not exactly free,
and, even if it's surely not a huge problem, by removing the copy ctor we
avoid any chance of it happening implicitly without being really needed.
If we do need to copy product_data some day, the copy ctor can always be
added back, this time with a clear explanation of why it is needed.

GC> But I don't like to remove tested code that may be useful someday.

 Finding simply deleted code is a challenge even with Git, but if you leave
a comment saying that this class is not copyable but could be easily made
so if needed and even used to be copyable in the past, it becomes trivial
to use git-blame to find the commit in which the copy ctor was removed and
then it's a matter of a simple git-revert to restore it.

GC> (My general practice is to run 'nychthemeral_test.sh' before each
GC> git-push; having run it, I decided to push with that questionable
GC> change, instead of waiting half an hour for another complete test
GC> without that change.)

 This is not directly related to the current topic, but I wonder if this
means that we should be running nychthemeral_test.sh in the CI builds
rather than just running the unit and GUI tests there as we do now...

GC> > GC> diff --git a/gpt_server.hpp b/gpt_server.hpp
GC> > GC> index cafeb177..f911087f 100644
GC> > GC> --- a/gpt_server.hpp
GC> > GC> +++ b/gpt_server.hpp
GC> > GC> @@ -48,6 +48,12 @@ class LMI_SO gpt_server final
GC> > GC>    public:
GC> > GC>      explicit gpt_server(mcenum_emission);
GC> > GC>  
GC> > GC> +    gpt_server(gpt_server const&) = default;
GC> > GC> +    gpt_server(gpt_server&&) = default;
GC> > GC> +    gpt_server& operator=(gpt_server const&) = delete;
GC> > GC> +    gpt_server& operator=(gpt_server&&) = delete;
GC> > GC> +    ~gpt_server() = default;
GC> > 
GC> >  Just to show that I'm not going to comment negatively on all changes in
GC> > this commit, this one is perfectly fine: the class can be copied/moved but
GC> > not assigned to (in any way).
GC> > 
GC> >  Although, somewhat spoiling the effect I was looking for, I think it 
might
GC> > still be nice to explain why do we want to forbid assigning to it...
GC> 
GC> Here, I want to consider a distinct question: if, for any given class,
GC> defaulted copy operations are correct, then can default assignment
GC> operations be incorrect?

 It's, of course, possible to maliciously design just such a class, but I
don't think it's something that can happen organically.

GC> It's a stage in my learning journey, not the final destination.

 Looking forward to, and good luck with, reaching the latter!
VZ

pgpvO1unzG_X7.pgp
Description: PGP signature