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

[Greg Chicares]

On 7/11/22 22:12, Vadim Zeitlin wrote:
> 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 [...]
[...]
> 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.

That's a valid concern, but isn't it already answered by declaring
it as 'private'?

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

My fear is that code rot will meanwhile have set in, so restoring it
will require retrofitting historical changes to the class. If we leave
it in place, it'll cost little extra to maintain it while we're making
such changes.

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

Certainly. I thought that was already being done. The only potential
concern I know of is that 'nychthemeral_test.sh' runs all architectures,
and for CI you might have them split; but something like
  lmi_toolchains="clang_gnu64" path/to/nychthemeral_test.sh
ought to be usable if you want to keep them separate.

BTW, this command
  make raze; time ./nychthemeral_test.sh 2>&1 |less
takes about thirty-five minutes here. It might take only half as long
without 'make raze', but at a time when compiler flags are experimentally
being changed, razing is necessary. Once compiler flags are stable,
I imagine that implementing 'ccache' will make razing almost free.

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

I was just looking at that one, so let me share the question I'm
pondering. This class has a data member:
    gpt_state state_                 {};
of a class that has custom copy operations (because defaulting them
would not work), and I've never even thought of implementing move
operations there. Should gpt_server be non-movable because it contains
a non-movable member, or does the language quietly substitute copy
semantics for that member so that a defaulted move of the owning
class is valid?

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

I stumbled on one case: const reference members. I suppose either
const-ness or reference-ness is enough to poison assignment.

Whether that's "organic" or not, the fact remains that I've done
it in a few places, so the question becomes whether to un-do it.