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

[Vadim Zeitlin]

On Sun, 10 Jul 2022 20:41:31 -0400 (EDT) Greg Chicares 
<gchicares@sbcglobal.net> wrote:

GC> branch: master
GC> commit 6a59da54d452e0e623714dfcdb48dabbff172f1d
GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> 
GC>     When in doubt, prefer the Rule of Five to the Rule of Zero

 Thanks for making these changes!

GC>     The Rule of Zero is fine for trivial classes, but few if any of these
GC>     classes are trivial. See:
GC>       https://lists.nongnu.org/archive/html/lmi/2022-07/msg00028.html
GC>     
GC>     This revision reverts the deletion of dtors, and adds the four other
GC>     special member functions as "= delete" if possible or "= default" if
GC>     necessary for compiling and linking. Inspecting the haphazard outcomes
GC>     suggests that this is not a good strategy, though the exercise was
GC>     illuminating.
GC>     
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.

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?

GC> diff --git a/fund_data.hpp b/fund_data.hpp
GC> index 15478273..96be4017 100644
GC> --- a/fund_data.hpp
GC> +++ b/fund_data.hpp
GC> @@ -48,6 +48,12 @@ class LMI_SO FundInfo final
GC>          ,std::string const& gloss = std::string()
GC>          );
GC>  
GC> +    FundInfo(FundInfo const&) = default;
GC> +    FundInfo(FundInfo&&) = default;
GC> +    FundInfo& operator=(FundInfo const&) = delete;
GC> +    FundInfo& operator=(FundInfo&&) = default;
GC> +    ~FundInfo() = default;

 This is another somewhat unusual configuration: if the class is not
assignable, even though it's copyable, why should it be move-assignable?

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

GC> diff --git a/product_data.hpp b/product_data.hpp
GC> index 0294af0d..237bcecd 100644
GC> --- a/product_data.hpp
GC> +++ b/product_data.hpp
GC> @@ -51,6 +51,12 @@ class glossed_string final
GC>          ,std::string const& gloss = std::string()
GC>          );
GC>  
GC> +    glossed_string(glossed_string const&) = default;
GC> +    glossed_string(glossed_string&&) = default;
GC> +    glossed_string& operator=(glossed_string const&) = default;
GC> +    glossed_string& operator=(glossed_string&&) = default;
GC> +    ~glossed_string() = default;
GC> +
GC>      glossed_string& operator=(std::string const&);
GC>  
GC>      bool operator==(glossed_string const&) const;
GC> @@ -91,7 +97,12 @@ class LMI_SO product_data
GC>    public:
GC>      explicit product_data(fs::path const& product_filename);
GC>      explicit product_data(std::string const& product_name);
GC> -    ~product_data() override;
GC> +
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.

GC> diff --git a/rounding_rules.hpp b/rounding_rules.hpp
GC> index 402dcb6d..9d986ef0 100644
GC> --- a/rounding_rules.hpp
GC> +++ b/rounding_rules.hpp
GC> @@ -49,6 +49,12 @@ class LMI_SO rounding_parameters final
GC>          ,std::string const& gloss = std::string()
GC>          );
GC>  
GC> +    rounding_parameters(rounding_parameters const&) = default;
GC> +    rounding_parameters(rounding_parameters&&) = delete;
GC> +    rounding_parameters& operator=(rounding_parameters const&) = default;
GC> +    rounding_parameters& operator=(rounding_parameters&&) = default;
GC> +    ~rounding_parameters() = default;

 This is another instance of weirdly move-assignable but not movable class.
It looks like this is actually intentional (once is a happenstance, but
twice is definitely a suspicious coincidence), but I still don't see why
would you want to do this and strongly believe that this merits an
explanatory comment because this is something so unusual that it certainly
provokes questions.

GC> diff --git a/rtti_lmi.hpp b/rtti_lmi.hpp
GC> index 00e4c5ac..17086548 100644
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;

 Same as above.

GC> diff --git a/stratified_charges.hpp b/stratified_charges.hpp
GC> index 5d0bac87..50b1a2ee 100644
GC> --- a/stratified_charges.hpp
GC> +++ b/stratified_charges.hpp
GC> @@ -78,6 +78,12 @@ class LMI_SO stratified_entity final
GC>          ,std::string const&         gloss = std::string()
GC>          );
GC>  
GC> +    stratified_entity(stratified_entity const&) = default;
GC> +    stratified_entity(stratified_entity&&) = delete;
GC> +    stratified_entity& operator=(stratified_entity const&) = default;
GC> +    stratified_entity& operator=(stratified_entity&&) = default;
GC> +    ~stratified_entity() = default;

 And again.


 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.

 Thanks in advance,
VZ

pgpiPchdjGAhG.pgp
Description: PGP signature