Re: [lmi] depr.impldec

[Greg Chicares]

On 7/13/22 23:39, Vadim Zeitlin wrote:
> 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.

Would it be generally regarded as a good rule if Sutter hadn't said it?

When we nod our heads in agreement with that rule, are we really embracing
the notion that a protected dtor must never be virtual? and if so, why?

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

Then it's not necessarily such a good rule.

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

The rule is good enough if we turn a blind eye, which we shouldn't do.

> GC> But instead of elaborating theoretically and creating confusion
> GC> through imprecision, let me propose a ready-to commit

[...amended in this message that was sent later...]

> On Wed, 13 Jul 2022 22:38:21 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
>  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.

One of us must take the opposite position if we're to get at the truth,
so let me say: this is ghastly. The changeset's 'git-diff --numstat':
  1       1       datum_base.hpp
  1       1       datum_boolean.hpp
  1       1       datum_string.hpp
  7       0       mc_enum.hpp
  7       0       tn_range.hpp
is perhaps acceptable if we gain some benefit from it (though I'll
question that presently). It could probably be smaller if we used the
Rule of Zero, but that's a separate consideration. (The changes sprawl
across numerous files, but we only have to apply them once, so that's
not too objectionable in itself.)

When you say
> It's a bit unusual to
> have the dtor become virtual in the middle of the inheritance chain
one might add "to put it mildly". I've never seen such a thing. I'd
be inclined to reject it in code review without a demonstration of
some actual benefit.

But I see no benefit at all. The illustrative patch below [0] compares
the 'sizeof' each class in two hierarchies that are minimal examples
of what 'datum_base' does--viz., it presents an interface with pure
virtuals. In one hierarchy, the base class's dtor is protected and
nonvirtual; in the other, it's protected and virtual, contrary to
the GoTW#5 advice AIUI. The classes are the same size (8 bytes here)
as opposed to an empty class (1 byte). They're the same size because
every one of them has a vtable, because every one has a pure virtual;
and making the dtor virtual or not doesn't change that.

For a base class like the erstwhile std::unary_function, a non-virtual
dtor is just fine. For a base class like 'datum_base', a non-virtual
dtor must change its nature to virtual along the inheritance chain,
which, lacking any benefit, is just gratuitous weirdness.

Instead of GoTW#5, I propose:

A base class destructor should generally be protected and virtual.
Exceptions: make it
 - public iff it is necessary to call the derived class's
   dtor though a pointer to base.
 - non-virtual iff that prevents the base from needing a vtable.

---------

[0] "The illustrative patch below":

--8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/sandbox_test.cpp b/sandbox_test.cpp
index 6baaf4c82..62c4ea157 100644
--- a/sandbox_test.cpp
+++ b/sandbox_test.cpp
@@ -23,7 +23,55 @@
 
 #include "test_tools.hpp"
 
+class A{};
+
+// Received doctrine:
+//   "A base class destructor should be either public and virtual,
+//   or protected and nonvirtual."
+
+// protected and nonvirtual
+class B0
+{
+  protected:
+    ~B0() = default;
+    virtual void pure_virt() = 0;
+};
+
+// protected and virtual, contrary to received doctrine
+class B1
+{
+  protected:
+    virtual ~B1() = default;
+    virtual void pure_virt() = 0;
+};
+
+class D0 : public B0
+{
+  public:
+    virtual ~D0() = default;
+    void pure_virt() override {}
+};
+
+class D1 : public B1
+{
+  public:
+    ~D1() override = default;
+    void pure_virt() override {}
+};
+
 int test_main(int, char*[])
 {
+    D0 d0;
+    D1 d1;
+    std::cout
+        << sizeof(A ) << " sizeof(A )\n"
+        << sizeof(B0) << " sizeof(B0)\n"
+        << sizeof(B1) << " sizeof(B1)\n"
+        << sizeof(D0) << " sizeof(D0)\n"
+        << sizeof(D1) << " sizeof(D1)\n"
+        << sizeof(d0) << " sizeof(d0)\n"
+        << sizeof(d1) << " sizeof(d1)\n"
+        << std::endl
+        ;
     return 0;
 }
--8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<--