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 12:12, Vadim Zeitlin wrote:
> On Sun, 10 Jul 2022 20:41:31 -0400 (EDT) Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
> 
> GC>     In class product_data, the copy ctor and copy assignment operator had
> GC>     been private, with the former implemented out of line and the latter
> GC>     deleted. The deleted member needn't be private. Perhaps the copy ctor
> GC>     should be private, or perhaps not; that will be considered next, but
> GC>     for now it's public.
> 
>  I suspect it might have remained private by mistake after the changes of
> c9dd0798c (Add and test product_data copy ctor, 2018-11-03) which didn't
> update its visibility after allowing it to be used. But I'm not completely
> sure neither.

Actually, it's used nowhere but in that unit test: if we remove the
copy ctor, everything builds correctly except for that unit test.
Commit c9dd0798c suggests that it was needed at one time, but
apparently it isn't needed any longer.

My first thought was to remove the copy ctor and the unit test.
But I don't like to remove tested code that may be useful someday.

Then I reconsidered the (only two) recent changes:

commit 702c4cdddc: Rule of Zero for auxiliary class glossed_string.
The only change to class product_data was initializing every data
member, which is a good change in its own right.

commit 6a59da54d4: Rule of s/Zero/Five/ . The intention was to
mitigate any negative effect the preceding commit might have had.
Although that commit hadn't changed class product_data (except for
the data-member initializations, which were incidental), I changed
that class, too, in ways that now seem regrettable, so I'll reverse
those changes to that class soon.

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

> GC> diff --git a/dbvalue.hpp b/dbvalue.hpp
> GC> index 6ee024b9..6e27f797 100644
> GC> --- a/dbvalue.hpp
> GC> +++ b/dbvalue.hpp
> GC> @@ -78,6 +78,12 @@ class LMI_SO database_entity final
> GC>          ,std::string const& gloss = std::string()
> GC>          );
> GC>  
> GC> +    database_entity(database_entity const&) = default;
> GC> +    database_entity(database_entity&&) = delete;
> GC> +    database_entity& operator=(database_entity const&) = default;
> GC> +    database_entity& operator=(database_entity&&) = default;
> GC> +    ~database_entity() = default;
> 
>  This is a very strange situation, I don't think there is ever any reason
> for having move assignment operator but not move ctor. Did you delete the
> latter intentionally?

Experimentally, for Rule of 0-->5 changes, I copied the old dtor that I
had removed, and added the other four members, all deleted; then I did
s/delete/default/ as few times as possible to get everything to build.
Here, "= delete" means only that the move ctor is not presently used.

This was valuable practice for me personally. At one point, I had
defaulted the copy operations and deleted the move operations, and I
had to think hard to understand why that wouldn't compile, when it
had compiled with none of those special members declared. I'm sure
you already know: move operations that are declared and deleted
participate in overload resolution, and that's why it's recommended
never to delete the "move" special members.

Having completed that learning exercise, I now need to go back and
make everything less irrational.

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

Here, I want to consider a distinct question: if, for any given class,
defaulted copy operations are correct, then can default assignment
operations be incorrect?

> GC> +    product_data(product_data const&); // Implemented out of line.
> 
>  Should we really write such comments for all non-inline member functions?
> It seems relatively unsurprising that a function not defined inline is
> implemented somewhere else, so I'm not sure if this comment adds anything
> useful.

I had already come to the same conclusion, and I've removed such
comments locally.

> GC> --- a/rtti_lmi.hpp
> GC> +++ b/rtti_lmi.hpp
> GC> @@ -125,6 +125,12 @@ class TypeInfo final
> GC>    public:
> GC>      TypeInfo(std::type_info const& z): ti_(&z) {}
> GC>  
> GC> +    TypeInfo(TypeInfo const&) = default;
> GC> +    TypeInfo(TypeInfo&&) = delete;
> GC> +    TypeInfo& operator=(TypeInfo const&) = default;
> GC> +    TypeInfo& operator=(TypeInfo&&) = default;
> GC> +    ~TypeInfo() = default;

You perceived that as another weird pattern, and suggested that it
needs at least a comment. I'd go farther and conclude that this
class's unit test is deficient because it builds, so the move ctor
is not yet tested.

>  Thanks again for these changes, they definitely make things more clear,
> but now that they are clear, I have more questions about why things are the
> way they are and I'd be curious to learn about your rationale.

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