Re: [lmi] depr.impldec

[Vadim Zeitlin]

On Wed, 13 Jul 2022 21:52:55 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> I propose that we follow Sutter's suggestion here:
GC>   
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-virtual
GC> | A base class destructor should be either public and virtual,
GC> | or protected and non-virtual

 This looks like a good rule.

GC> and, where it makes sense, prefer the "protected" path because
GC> it's stricter.

 Yes, certainly, for some meaning of "making sense".

[...deleting a class without virtual dtor via a base pointer...]
GC> Unless we use "protected" to make those problems impossible.

 Not quite, it's possible for the class -- or a derived class -- to delete
itself by doing "delete this" and it's also possible for a static class
function to delete some object of this class even though it has protected
dtor (it's also possible to delete another object from a non-static member
function, of course, but IME this is even rarer). But the risk of this
happening is much smaller, of course.

 Generally speaking, it's almost unfair how much less you can care about
the potential misuse of your code when writing an application, rather than
a library.

GC> But instead of elaborating theoretically and creating confusion
GC> through imprecision, let me propose a ready-to commit change
GC> (for which I don't apply the Rule of Six text-template proposed
GC> in my last message, only because this way the diff is smaller
GC> and easier to read):
GC> 
GC> --8<----8<----8<----8<----8<----8<----8<----8<--
GC> diff --git a/datum_base.hpp b/datum_base.hpp
GC> index 8bebc63fc..c57aca38e 100644
GC> --- a/datum_base.hpp
GC> +++ b/datum_base.hpp
GC> @@ -31,20 +31,20 @@
GC>  class LMI_SO datum_base
GC>  {
GC>    public:
GC> -    datum_base() = default;
GC> +    void enable(bool);
GC> +    bool is_enabled() const;
GC>  
GC> +    virtual std::istream& read (std::istream&)       = 0;
GC> +    virtual std::ostream& write(std::ostream&) const = 0;
GC> +
GC> +  protected:
GC> +    datum_base() = default;
GC>      datum_base(datum_base const&) = default;
GC>      datum_base(datum_base&&) = default;
GC>      datum_base& operator=(datum_base const&) = default;
GC>      datum_base& operator=(datum_base&&) = default;
GC>      virtual ~datum_base() = default;
GC>  
GC> -    void enable(bool);
GC> -    bool is_enabled() const;
GC> -
GC> -    virtual std::istream& read (std::istream&)       = 0;
GC> -    virtual std::ostream& write(std::ostream&) const = 0;
GC> -
GC>    private:
GC>      bool enabled_ {true};
GC>  };
GC> 
GC> --8<----8<----8<----8<----8<----8<----8<----8<--
GC> 
GC> We never instantiate a datum_base, and we never want to,
GC> so all special member functions can be "protected".
GC> We can still form a pointer to a datum_base, but we
GC> can't delete through it because there's no public
GC> dtor that would permit that. All our compilers accept
GC> this; UBSan doesn't complain about it, either.
GC> 
GC> What do you think?

 Yes, this looks good...

On Wed, 13 Jul 2022 22:38:21 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 7/13/22 21:52, Greg Chicares wrote:
GC> [...]
GC> > let me propose a ready-to commit change
GC> 
GC> ...which left the dtor virtual.

... except I was going to ask exactly about this until I read the next
message. Luckily I read it first, so now I'm not going to ask.

GC> With the changes below, it actually does compile.

 I don't have any real comments about these changes. It's a bit unusual to
have the dtor become virtual in the middle of the inheritance chain (as in
e.g. datum_base -> datum_string -> datum_sequence), but I can't find
anything really wrong with this.

 Regards,
VZ

pgpmjUDsSmxAX.pgp
Description: PGP signature