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

[Vadim Zeitlin]

On Tue, 12 Jul 2022 16:18:34 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 7/11/22 22:12, Vadim Zeitlin wrote:
GC> > On Mon, 11 Jul 2022 16:22:14 +0000 Greg Chicares 
<gchicares@sbcglobal.net> wrote:
GC> > 
GC> > GC> On 7/11/22 12:12, Vadim Zeitlin wrote:
GC> > GC> > On Sun, 10 Jul 2022 20:41:31 -0400 (EDT) Greg Chicares 
<gchicares@sbcglobal.net> wrote:
GC> > GC> > 
GC> > GC> > GC>     In class product_data, the copy ctor and copy assignment 
operator [...]
GC> [...]
GC> > GC> My first thought was to remove the copy ctor and the unit test.
GC> > 
GC> >  I agree with this, it doesn't seem necessary to allow the objects of this
GC> > class to be copied and, being rather big, copying them is not exactly 
free,
GC> > and, even if it's surely not a huge problem, by removing the copy ctor we
GC> > avoid any chance of it happening implicitly without being really needed.
GC> 
GC> That's a valid concern, but isn't it already answered by declaring
GC> it as 'private'?

 This wouldn't help if the copying happened inside a member function of
this class itself, but it does significantly restrain the scope of the
problem, of course.

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

 No, it isn't, the testing steps are

- make check_concinnity check_physical_closure
- make unit_tests (or "make check" for the autotools builds)
- run the GUI test (for MSW build only)

 I think this was all that nychthemeral_test.sh did when I added the CI
script originally, but many things have been added to the former since then
and it would probably be better to reuse it rather than to integrate them
piecemeal into the CI script.

 OTOH there is some stuff in .github/workflows/ci.yml which is quite
helpful for the CI builds but not really necessary for the local ones.

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

 Thanks for the hint!

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

 You don't lose anything by starting to use ccache right now, it considers
compiler flags as part of the cache key and won't reuse the previously
compiled file if the flags have changed (I don't know if it's smart enough
to filter out immaterial changes to the flags, but it's definitely so dumb
as to not take them into account at all).

GC> > GC> > GC> diff --git a/gpt_server.hpp b/gpt_server.hpp
GC> > GC> > GC> index cafeb177..f911087f 100644
GC> > GC> > GC> --- a/gpt_server.hpp
GC> > GC> > GC> +++ b/gpt_server.hpp
GC> > GC> > GC> @@ -48,6 +48,12 @@ class LMI_SO gpt_server final
GC> > GC> > GC>    public:
GC> > GC> > GC>      explicit gpt_server(mcenum_emission);
GC> > GC> > GC>  
GC> > GC> > GC> +    gpt_server(gpt_server const&) = default;
GC> > GC> > GC> +    gpt_server(gpt_server&&) = default;
GC> > GC> > GC> +    gpt_server& operator=(gpt_server const&) = delete;
GC> > GC> > GC> +    gpt_server& operator=(gpt_server&&) = delete;
GC> > GC> > GC> +    ~gpt_server() = default;
GC> > GC> > 
GC> > GC> >  Just to show that I'm not going to comment negatively on all 
changes in
GC> > GC> > this commit, this one is perfectly fine: the class can be 
copied/moved but
GC> > GC> > not assigned to (in any way).
GC> > GC> > 
GC> > GC> >  Although, somewhat spoiling the effect I was looking for, I think 
it might
GC> > GC> > still be nice to explain why do we want to forbid assigning to it...
GC> 
GC> I was just looking at that one, so let me share the question I'm
GC> pondering. This class has a data member:
GC>     gpt_state state_                 {};
GC> of a class that has custom copy operations (because defaulting them
GC> would not work), and I've never even thought of implementing move
GC> operations there. Should gpt_server be non-movable because it contains
GC> a non-movable member,

 I think it's already non-movable (which is something not immediately
obvious from the diff above).

GC> or does the language quietly substitute copy semantics for that member
GC> so that a defaulted move of the owning class is valid?

 Yes, but perhaps not in the way you meant it: initializing gpt_server from
an rvalue reference to gpt_server will use copy, but just because it will
use its copy ctor. This happens because "= default" on the move constructor
above actually _deletes_ it, because it can't be implemented, and such
deleted-due-to-default move ctor is ignored by the overload resolution, so
the copy ctor will be found and used instead.

 Considering all this, I think it would be more clear to delete the move
ctor instead. Alternatively, the class could be made actually movable,
but this doesn't seem to be especially useful for this class.

GC> > GC> Here, I want to consider a distinct question: if, for any given class,
GC> > GC> defaulted copy operations are correct, then can default assignment
GC> > GC> operations be incorrect?
GC> > 
GC> >  It's, of course, possible to maliciously design just such a class, but I
GC> > don't think it's something that can happen organically.
GC> 
GC> I stumbled on one case: const reference members. I suppose either
GC> const-ness or reference-ness is enough to poison assignment.

 Either is enough for the assignment not to be possible, but for me this
doesn't mean it's incorrect -- it's disallowed, which is something quite
different (and not nearly as bad). My answer above was to the question of
whether a class with a working defaulted copy ctor could have a broken
default assignment operator and I still think that this shouldn't happen.
Sorry if I had misunderstood your question...

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

 Sorry, I'm not sure what exactly do you propose to undo (or not). All I
can say is that a class with const or reference fields (or both) can't be
assignable, so, according to the general principle, it's better to be
explicit about it and delete the corresponding special member functions.

 Please let me know if I misunderstood you again,
VZ

pgpJg7Y9E0B7q.pgp
Description: PGP signature