Contra P0722R0 "destroying operator-delete"

[Arthur O'Dwyer]

This is related to Richard Smith and Andrew Hunter's P0722R0 "Controlling destruction in delete expressions".
The paper begins this way:

Consider the following class:

class inlined_fixed_string {
  public:
   inlined_fixed_string() = delete;
   const size_t size() const { return size_; }

   const char *data() const {
     return static_cast<const char *>(this + 1);
   }

   // operator[], etc, with obvious implementations

   inlined_fixed_string *Make(const std::string &data) {
     size_t full_size = sizeof(inlined_fixed_string) + data.size();
     return new(::operator new(full_size))
                  inlined_fixed_string(data.size(), data.c_str());
   }

  private:
   inlined_fixed_string(size_t size, const char *data) : size_(size) {
     memcpy(data(), data, size);
   }
   size_t size_;
};

This defines what is (effectively) a variable-sized object, using that to
implement an array of size determined at runtime while saving a pointer
indirection. (Note: this pattern is simpler (and generally written) with
flexible array members, despite their being nonstandard.)

However, what happens when we delete such a string s in the presence of
sized-delete?

Well, you get misbehavior, of course. Syntactically, C++ allows you to use "delete" on any pointer; but semantically, you should use "delete" only on pointers that were originally obtained from "new". And in your case, you didn't obtain the pointer from "new"; you obtained it from the public factory function "Make".

This code is broken because it has a public "Make" factory and private constructor, but it is missing a public "Destroy" factory and private destructor. If you rewrite it to use that idiom, then the problem goes away.

The corrected code looks like this, and requires absolutely no core language changes.

class inlined_fixed_string {
  public:
   inlined_fixed_string() = delete;
   size_t size() const { return size_; }

   char *data() {
     return reinterpret_cast<char *>(this + 1);
   }

   // operator[], etc, with obvious implementations

   static inlined_fixed_string *Make(const std::string &data) {
     size_t full_size = sizeof(inlined_fixed_string) + data.size();
     return new(::operator new(full_size))
                  inlined_fixed_string(data.size(), data.c_str());
   }

   static void Destroy(inlined_fixed_string *p) {
     size_t full_size = sizeof(*p) + p->size();
     p->~inlined_fixed_string();
     ::operator delete(p, full_size);
   }
  private:
   inlined_fixed_string(size_t n, const char *s) : size_(n) {
     memcpy(data(), s, n);
   }
   ~inlined_fixed_string() {}
   size_t size_;
};

int main() {
    inlined_fixed_string *s = inlined_fixed_string::Make("hello world");
    inlined_fixed_string::Destroy(s);

    std::shared_ptr<inlined_fixed_string> p(inlined_fixed_string::Make("hello shared"), inlined_fixed_string::Destroy);
}

That said, if you do pursue "destroy and delete in one atomic operation", it will be of especial interest to the garbage-collection and RCU folks. Louis Dionne is interested in building a deferred_reclamation_allocator<T> that can defer calls to destroy() and deallocate() in pairs, which is essentially the primitive you wanted to provide in P0722R0. (But again, you don't need P0722R0, and I'd much much rather the Committee not pursue it. Nobody understands new/delete as it is; let's not make the situation even worse out of a misbegotten wish to "delete" objects we haven't "new"ed.)

Incidentally, if the individual words didn't already have domain-specific meanings, I would love to describe inlined_fixed_string's semantics in terms of the Make "factory function" and the Destroy "glue factory function". ;)

my $.02,
–Arthur

[Richard Smith]

Please read to the end of the paper; that alternative is discussed and we explain why it is not entirely satisfactory.

[Thiago Macieira]

On terça-feira, 17 de outubro de 2017 10:02:11 PDT 'Richard Smith' via ISO C++

Standard - Future Proposals wrote:
> Please read to the end of the paper; that alternative is discussed and we
> explain why it is not entirely satisfactory.

And I agree. My first reaction was, "why not just add a class operator delete",
which is answered in the paper.

I like the idea of the destroying operator delete.

Can we have a constructing operator new too? That is, the dual of what the
paper proposes. That would allow for the object to have a public constructor
too. This has been a request in QObject for some time, but we have never found
a satisfactory (i.e., non-hacky or inefficient) solution.

Almost all class types using private implementations would benefit from this.
Take this from actual code <https://code.woboq.org/qt5/qtbase/src/corelib/
kernel/qobject.cpp.html#_ZN7QObjectC1EPS_>:

QObject::QObject(QObject *parent)
: d_ptr(new QObjectPrivate)

You'll find that QObject is roughly even divided between stack / member objects
and those created with new QObject (or one of the derived classes).

It would be grand to have:

QObject *QObject::operator new(/* implicit size */)
{
size_t full_size = sizeof(QObject) + sizeof(QObjectPrivate);
auto obj = static_cast<QObject *>(::operator new(full_size));
auto d = reinterpret_cast<QObjectPrivate *>(obj + 1);
// call a private or protected constructor
return new (obj) QObject(InPlaceTag{}, d);
}

Coupled with a member operator delete (not necessarily the destroying delete),
the only other things we need is 1 bit indicating that the main class does not
own the private object and should not delete it.

--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
Software Architect - Intel Open Source Technology Center

[Arthur O'Dwyer]

On Tue, Oct 17, 2017 at 10:02 AM, Richard Smith <richar...@google.com> wrote:

On Tue, 17 Oct 2017, 09:39 Arthur O'Dwyer, <arthur....@gmail.com> wrote:

This defines what is (effectively) a variable-sized object, using that to
implement an array of size determined at runtime while saving a pointer
indirection. (Note: this pattern is simpler (and generally written) with
flexible array members, despite their being nonstandard.)

However, what happens when we delete such a string s in the presence of
sized-delete?

Well, you get misbehavior, of course. Syntactically, C++ allows you to use "delete" on any pointer; but semantically, you should use "delete" only on pointers that were originally obtained from "new". And in your case, you didn't obtain the pointer from "new"; you obtained it from the public factory function "Make".

This code is broken because it has a public "Make" factory and private constructor, but it is missing a public "Destroy" factory and private destructor. If you rewrite it to use that idiom, then the problem goes away.

Please read to the end of the paper; that alternative is discussed and we explain why it is not entirely satisfactory.

I saw the end of the paper, but I interpreted it as just a continuation of the mistaken idea that it's okay to "delete" an object that was not created with "new". P0722R0 ends like this:

The obvious and natural alternative to this proposal is to destroy and delete
objects with custom deletion semantics by using special-case logic, instead
of using 'operator delete'. This has a number of disadvantages:

 * It violates the principle that user-defined types are used like built-in
   types: as soon as you use more advanced allocation techniques, you don't
   get to use delete expressions any more.

Yes, this is accurate. The programmer should never expect "delete expressions" to work with objects that were not created via "new expressions".  The programmer should always expect that if a pointer is gotten from a "factory", then it should be returned to the corresponding "glue factory" when the programmer is done with it. You can use RAII to ensure that this happens. One built-in RAII approach is to use std::shared_ptr as in my corrected example.  Another approach is to have the "factory" return a value-semantic wrapper around the polymorphic object, in which case you have exactly type-erasure á là std::function.

Sometimes the appropriate "glue factory" is "delete p", but it should be unsurprising that sometimes the appropriate "glue factory" is not "delete p". As soon as your "allocation technique" becomes more advanced than "p = new T(...)", you should expect that your "deallocation technique" will have to be more advanced than "delete p".

 * Existing class hierarchies with virtual destructors cannot be transparently
   extended with derived classes that allocate dynamic leading or trailing
   storage.

Mmmm, I'd say yes they can (although it's not a good idea). If the existing class hierarchy provides a "factory" function for creating objects of the polymorphic type, then it would make sense for the derived class to try to "override" that factory function, and to override the corresponding "glue factory" function for destruction-and-deallocation. If the class author intended for the "glue factory" function to be overridden, they'll have made it a virtual method:

   virtual void Destroy() {

     inlined_fixed_string *p = this;

     size_t full_size = sizeof(*p) + p->size();

     p->~inlined_fixed_string();

     ::operator delete(p, full_size);

   }

However, this is fundamentally different from a destructor alone. A derived class's destructor implicitly calls its base's destructor (there's no way around that in C++ today); but a derived class's Destroy() must not call its base's Destroy(). This suggests that the derived class must itself know everything about how to clean up its base; which means that this is probably not a good fit with classical OOP.

Can you imagine a derived class extending inlined_fixed_string?  How would it look and act?  Especially, how would I construct and destroy instances of the derived class?

 * The local choice of deallocation strategy leaks out to clients of the code.
   For example, a custom deleter must be specified when using unique_ptr<T>,
   and make_unique and make_shared can't be used any more.

Note that the standard requires specializations of default_delete<T> to have
the same effect as calling "delete p;", so specializing default_delete is not
a correct alternative in C++17. We could lift that restriction, but that would
not help for other (perhaps user-defined) resource management types that use
new and delete to manage objects.

The last bullet and its subsequent paragraph seem to hold two competing views of how-and-whether to extend C++. The bullet says, "We want to use 'delete' on factory-made pointers and have it Just Work. We must change the language to make that syntax work!"  But the paragraph says, "You want to use 'default_delete' on factory-made pointers and have it Just Work? No, that would just complicate the language."

Whereas my attitude is "You want to use anything other than the corresponding factory method to deallocate factory-made pointers and expect it to work? Just say no."

–Arthur

[Andrew Hunter]

On Tue, Oct 17, 2017 at 11:30 AM, Arthur O'Dwyer
<arthur....@gmail.com> wrote:
> Yes, this is accurate. The programmer should never expect "delete
> expressions" to work with objects that were not created via "new
> expressions".

Which programmer?

Suppose I have 10,000 programmers who make use of a particular
string-like class (as a matter of fact, I actually do.) There are only
a very few sources of this object, but plenty of consumers. It
currently uses no hanky-panky in allocation; therefore everyone
happily deletes them, stores them in unique_ptr, and so on. There is
X00 kLOC of code doing this. Now someone realizes that this class, on
a number of hot paths, would benefit from inlining allocation as in
the example. We can either rewrite every line of code, and train
every programmer to only use Destroy, and enforce use of
custom-deleter unique_ptrs, and ban every container that doesn't have
full support for that...or we can make "delete s" do the right thing.

As the paper points out, this is in fact already possible to do;
defining operator delete on the type works and is well-defined. But
it gives up efficiency in a sized-delete world.

> Sometimes the appropriate "glue factory" is "delete p", but it should be
> unsurprising that sometimes the appropriate "glue factory" is not "delete
> p".

I do not think this is an argument against making it possible to have
that deallocation function be "delete p" where we can.

> * The local choice of deallocation strategy leaks out to clients of the
> code.
> For example, a custom deleter must be specified when using unique_ptr<T>,
> and make_unique and make_shared can't be used any more.
>
> Note that the standard requires specializations of default_delete<T> to have
> the same effect as calling "delete p;", so specializing default_delete is
> not
> a correct alternative in C++17. We could lift that restriction, but that
> would
> not help for other (perhaps user-defined) resource management types that use
> new and delete to manage objects.
>
> The last bullet and its subsequent paragraph seem to hold two competing
> views of how-and-whether to extend C++. The bullet says, "We want to use
> 'delete' on factory-made pointers and have it Just Work. We must change the
> language to make that syntax work!" But the paragraph says, "You want to
> use 'default_delete' on factory-made pointers and have it Just Work? No,
> that would just complicate the language."

You may be misreading that section? We are saying that without P0722
or a similar fix, it is not possible to make default_delete work, as
overriding it would violate the spec. And while changing *that* rule,
instead of adding the feature we propose, would solve the problem for
std::unique_ptr<T> specifically, it would do nothing for all the
*other* possible users.

> Whereas my attitude is "You want to use anything other than the
> corresponding factory method to deallocate factory-made pointers and expect
> it to work? Just say no."
>

I think you are perhaps not seeing the proper perspective: we are
trying to widen the already large set of cases where "delete p" *is*
the appropriate factory deletion method. This is already an extremely
common pattern, and one with numerous advantages (the paper specifies
some of them.) Remember in particular that there are many reasons
where factory-creation is a useful pattern without any funny business
happening on the allocation (the trivial example being creation of an
object chosen dynamically from some virtual hierarchy.) Are you
saying that

Widget *Widget::Make(string widget_spec);

is badly designed unless I always also specify Widget::Destroy? (If
so, why do we have virtual destructors at all?) We simply want to be
able to use delete as our "factory deallocation method" in some
additional cases.

[Arthur O'Dwyer]

On Tue, Oct 17, 2017 at 12:39 PM, Andrew Hunter <a...@google.com> wrote:

On Tue, Oct 17, 2017 at 11:30 AM, Arthur O'Dwyer
<arthur....@gmail.com> wrote:
> Yes, this is accurate. The programmer should never expect "delete
> expressions" to work with objects that were not created via "new
> expressions".

Which programmer?

All of them. ;)

Are you saying that

    Widget *Widget::Make(string widget_spec);

is badly designed unless I always also specify Widget::Destroy?

Yes. In fact, I would say that this function-returning-an-owning-raw-pointer is badly designed regardless of the existence of Widget::Destroy(), because the raw pointer does not say anything about its ownership and violates RAII. I would rather have

    Widget Widget::Make(string widget_spec);

(value-semantics) or at least

    std::shared_ptr<Widget> Widget::Make(string widget_spec);

(handle-semantics, but with the appropriate "destroyer" bundled alongside the pointer).

 

(If so, why do we have virtual destructors at all?)

Classical polymorphism. You could actually make the virtual destructor Do The Right Thing in this case, but that would require that your object instance know its own dynamic type. This type-safety is the cornerstone of classical OOP. In C++ we would write something like this:

class fixed_string {

public:

    static fixed_string *Make(const std::string &data) {

        assert(data.size() < 256);

        return make_impl(std::make_index_sequence<256>{}, data);

    }

    

    template<size_t... Is>

    static fixed_string *make_impl(std::index_sequence<Is...>, const std::string &data);

    

    virtual ~fixed_string() = default;

protected:

    size_t size_;

};

template<size_t N>

class inlined_fixed_string : public fixed_string {

    char data_[N ? N : 1];

public:

    inlined_fixed_string(const char *s) {

        size_ = N;

        memcpy(data_, s, N);

    }

};

template<size_t... Is>

fixed_string *fixed_string::make_impl(std::index_sequence<Is...>, const std::string &data) {

    using FP = fixed_string* (*)(const char *);

    FP arr[] = {

        +[](const char *s) -> fixed_string* { return new inlined_fixed_string<Is>(s); } ...

    };

    return arr[data.size()](data.data());

}

Now we have a base class with a virtual destructor, and a family of derived classes all of which override that virtual destructor to do the correct cleanup for each respective derived class. We cannot create new derived classes at runtime — we must specify at compile-time the complete set of derived classes that we care about — but this is just the usual cost of type-safety. It's the same reason there's no std::visit(std::any).

Again, I would probably argue that returning raw owning pointers in the classical-OOP fashion is a bad idea, and I'd encourage wrapping things up in value-semantic wrappers and not using inheritance at all (at least not in a way that's visible to the user-programmer). But if you must use classical-OOP and raw owning pointers, then classical-OOP does provide some tools for each derived instance to know how to destroy itself.

–Arthur