Re: [lmi] depr.impldec

[Greg Chicares]

On 7/14/22 13:56, Vadim Zeitlin wrote:
> On Thu, 14 Jul 2022 13:11:05 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 7/13/22 23:39, Vadim Zeitlin wrote:
> GC> > On Wed, 13 Jul 2022 21:52:55 +0000 Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
> GC> > 
> GC> > GC> I propose that we follow Sutter's suggestion here:
> GC> > GC>   
> http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-virtual
> GC> > GC> | A base class destructor should be either public and virtual,
> GC> > GC> | or protected and non-virtual

Summary of snipped part: I applied that rule to the hierarchy rooted in
class datum_base, which has a couple of pure virtuals. I found that
making the base class dtor non-virtual didn't make its 'sizeof' smaller
because the pure virtuals still require a vtable; but I needed to make
some of the derived classes' dtors virtual, e.g.:
-    ~datum_string() override = default;
+    virtual ~datum_string() = default;

[Note: though not others', e.g.:
-    ~datum_boolean() override = default;
+    ~datum_boolean() = default;
but the difference a consequence of the OOP sin you point out below.
--end note]

and it seems really weird to change the virtuality of the dtor when
moving up the inheritance chain. More weirdness, no benefit, so...

> GC> I'd be inclined to reject it in code review without a demonstration of
> GC> some actual benefit.
> 
>  It's true that I don't see much of it. From efficiency point of view, the
> gains are certainly negligible. From the correctness point of view, it
> prevents from using objects via pointers to the base class, but it's not
> really clear why is it an advantage.

If we later want to change it back (make the base dtor virtual), then
we'd better remember to change every class. Modern compiler with strong
warnings available will probably help us, or we could use git to revert
the weird change and that'll probably work, but it seems like a poor
idea to introduce weirdness with only a probabilistic likelihood of
being able to back it out correctly later.

Now the scorecard becomes: more weirdness, no benefit, a restriction
on a capability that could be useful, and an imperfect likelihood that
we could revert everything later, with effort.

Oh, and you also pointed out that making the improvidently non-virtual
dtor doesn't provide a firm guarantee that it can't be called except
by a derived-class dtor. That probably wouldn't happen, but there are
too many "probablies" here.

>  In fact, maybe it's worth taking a step back and asking ourselves why
> exactly do we want to prevent this from being done. I don't see anything
> really wrong with having a vector<unique_ptr<datum_base>> for example, is
> there?

I see no reason why that would not work. The question is whether the
Principle of Least Privilege (PoLP) is sufficient reason to limit
access as severely as is consistent with the design purpose.
I tend to think that it is.

> GC> For a base class like the erstwhile std::unary_function, a non-virtual
> GC> dtor is just fine. For a base class like 'datum_base', a non-virtual
> GC> dtor must change its nature to virtual along the inheritance chain,
> GC> which, lacking any benefit, is just gratuitous weirdness.
> 
>  Maybe the real problem is that it becomes virtual, i.e. perhaps there is
> something wrong with our inheritance hierarchy and we should avoid having a
> class (datum_string) which is both a concrete class and a base class?

Meyers, MEC++ #33: "Make non-leaf classes abstract".

My first reaction is that I would never assign a Lizard to a Chicken
through a pointer to Animal, because I zealously avoid pointers.

Except...

template<> struct reconstitutor<datum_base,Input>
{
    typedef datum_base DesiredType;
    static DesiredType* reconstitute(any_member<Input>& m)
        {
        DesiredType* z = nullptr;
        z = exact_cast<datum_string            >(m); if(z) return z;
        z = reconstitutor<datum_sequence,Input>::reconstitute(m); if(z) return 
z;
        z = reconstitutor<mc_enum_base  ,Input>::reconstitute(m); if(z) return 
z;
        z = reconstitutor<tn_range_base ,Input>::reconstitute(m); if(z) return 
z;
        return z;
        }
};

So my second reaction is that it looks like I might have taken
adequate precautions, by (in effect) recapitulating the class
hierarchy in reconstitute(). And it always works, or at least
I'm unaware of any case where it doesn't...as long as no one
foolishly changes the order above so that the exact_cast case
isn't treated first.

Fixing this is simple, at least in concept: split datum_string
into an abstract datum_string_base and a concrete class that
would use the original "datum_string" name. If you had suggested
that about twenty-five years ago (before we met), I would most
likely have embraced the idea, and written reconstitutor()
otherwise.

The question now is whether we should touch this and possibly
break it, knowing that testing a deep change may require deep
thought. It's been in production for decades and no one has
ever reported a problem stemming from this OOP sin.

With a twinge of remorse, I must say I feel disinclined to
pursue it.

> GC> Instead of GoTW#5, I propose:
> GC> 
> GC> A base class destructor should generally be protected and virtual.
> GC> Exceptions: make it
> GC>  - public iff it is necessary to call the derived class's
> GC>    dtor though a pointer to base.
> GC>  - non-virtual iff that prevents the base from needing a vtable.
> 
>  So, equivalently:
> 
> - For a class with virtual functions, make the dtor virtual.
> - Make it public by default or protected if using objects via the base
>   class pointer is forbidden.

That's inequivalent in one respect: I'd default to protected,
calling the PoLP sufficient justification; you'd default to
public, because...

>  But I still struggle to come up with any good example of a situation when
> we need to forbid doing this, to be honest. So maybe we should just always
> make the base class dtor public (and then, unavoidably, virtual) and forget
> about all the complications?

I have a pending PoLP commit--it's the first patch I attached in
this thread, which claimed to make a base class dtor non-virtual
but actually did no such thing. I can push it now, or discard it.
I'll commit it only if it gets a majority in favor, you and I
being the eligible voters. Here's my case in favor:

- PoLP, but I already said that.

- "Make non-leaf classes abstract": if we were to follow through
with that idea, wouldn't we make the special members of non-leaf
classes (including datum_base) 'protected'? In that case, pushing
that patch is at least a first, small, incremental step in the
right direction.

And here's my case against:

- This rule, for which I'll give a URL because it's an Americanism:
  https://en.wikipedia.org/wiki/Pottery_Barn_rule
It may seem that making the base class's members 'protected'
is perfectly safe, because the compiler would warn us of any
actual usage that's broken thereby. We can detect any breakage
at compile time...as long as we don't do anything that would
move potential errors to run time...oops: 'reconstitutor'.
The risk is not obviously less than refactoring datum_string
into an abstract and a complete class. If we're going to run
the risk, or rather pay the cost to satisfy ourselves that
our change is correct, then we should do that refactoring;
else, we should do nothing.

I'm not ready to cast a vote yet. What do you think?
Maybe I should at least do the refactoring and present
it here for discussion: splitting a class is easy, and
the hard part is ascertaining correctness.