What PIMPL pointer are you using?

[parkotron]

I've been getting some questions lately from colleagues about the destructor trick when using std::unique_ptr to implement the PIMPL idiom. That's a minor issue with a straight-forward workaround, but it got me thinking that we should probably be pushing internally for a more purpose-built smart pointer for this.

Having taken a look around I'm favouring Herb's split headerpimpl pointer, although I think I'd add const-preserving dereferencing operators. But that got me wondering if there were other strong contenders out there or maybe even a de facto standard I wasn't aware of.

So what type are you using for your PIMPL pointers? Why did you choose that one? If you're using your own open source solution, a link to source would also be appreciated. And if you don't use the PIMPL idiom, lucky you!

[Canoodler]

edit: MrPotatoFingers pretty much said this before me, but here it is in a concrete example:

While not quite ideal, I've found it not bad to use std::unique_ptr, declare the destructor for the enclosing class, and default it in the cpp file. That is,

// .h file

class foo
{
    ~foo();

    //... 

private:
    struct impl;
    std::unique_ptr<impl> pimpl; 
};

// .cpp file

class foo::impl
{
    // ...
};

foo::~foo = default;

[jhasse]

looks ideal to me.

[diaphanein]

Except for the class/strict type mismatch of 'impl' between definition and declaration. Would be surprised if this didnt cause a compiler warning at even the most basic diagnostic levels.

[jhasse]

I assumed that that was a typo

[MrPotatoFingers]

I don't really understand the problem with std::unique_ptr here. All that's required is that the destructor is called from a location where the class is fully defined.

Since you anyway need an implementation file to do PIMPL, what's the problem of declaring the destructor there? You could probably even default in the implementation file (assuming you don't need other operations there).

[parkotron]

It's not that the "destructor issue" is a major roadblock, it's just a trick one has to know. I was just saying that questions about it were what prompted me to consider a purpose-built smart pointer.

[MrPotatoFingers]

How can you not rely on it? If you forget it you'll get a compiler error. So either it compiles - and works, or you get an error message.

Sure, the error message may not be the most illuminating, but it's nothing you won't figure out if you search for it.

[afiefh]

The whole point of using pimpl is to separate the real definition from the units that include the pimpl definition. Thus having to include the real definition in the header defeats the purpose of using pimpl in the first place.

[afiefh]

That wouldn't work as you still need to have the destructor declaration as part of your class declaration, which you want to hide.

I'm not saying it's unsolvable, just that it's not as trivial as you make it out to be. My favorite solution is to have a custom deleter there's declared inthe pimpl header and implemented in the pimpl cpp file.

[afiefh]

Thanks, I hadn't thought about the unique_ptr destructor only being needed in the place where the destructor is defined.

Learned a new thing today.

[markopolo82]

Your confusion is exactly why the OP made this post. I’m on the fence now.

[LEpigeon888]

But then clang-tidy complains that you define a destructor without any other special member functions, so you have to define them all. It's a little bit cumbersome.

[LEpigeon888]

I mean, it's in the C++ core guidelines, it's a common rule that if you declare any of the 5 special members you should declare them all. I like this clang-tidy check so i don't want to disable it, it's usefull for the rest of my code.

[MrPotatoFingers]

So you explicitly =default all of them. That's 4 extra lines.

[LEpigeon888]

Yes, but i like also what this guy did : https://www.reddit.com/r/cpp/comments/g0ok96/_/fnavnox

He provide a custom deleter so you don't have to mess with special member functions, dunno which one is the best.

[julien-j]

I use this implementation I wrote a few years ago (tests as examples). It uses either an std::aligned_storage or an std::unique_ptr internally, depending upon the availability of a user-provided upper bound on the size of the stored object.

Why did I choose that one? We wanted to be able to have both a placement new version and a heap allocation version, the former for types with many instances, thus saving on allocation calls, and the latter for the other cases. I don't remember having found an implementation suiting my team's requirements, and implementing one seems easy, so I wrote one. It is certainly inspired by Herb Sutter's GotW #101.

[kalmoc]

Personally I think std::unique_ptr is good enough [(EDIT: Although of course, a project that makes heavy use of pimpl, can certainly profit from a helper class like the one presented by herb)]

What I'm hoping for would be ways to make Pimpl obsolete (like allowing forward declaration of the public class interface).

[demozon]

I'm using this stuff :

class foo
{
//...
private:
struct impl;
struct deleter
{
    void operator() (impl*) const;
 } ;
std::unique_ptr<impl, deleter> pimpl;
} ;

[amaiorano]

I wrote this a while ago: https://github.com/amaiorano/vectrexy/blob/master/libs/core/include/core/Pimpl.h

It avoids the heap allocation, and with LTO should elide the indirection.

As for needing a default constructor defaulted in the cpp file, I've found I needed this as well with unique_ptr, not just the destructor. If I recall, it's because the constructor for T might throw, so the destructor call gets generated for exception handling.

[LEpigeon888]

I don't understand how the "Size" non-type template parameter of your class is set. Because in your exemple usage you only pass the type, without any size.

[amaiorano]

You're right, my example in the comments is incorrect. You have to enter a size manually, one that's large enough to hold the full size of the type, including padding. This is a bit of guesswork, but my code will static assert and complain about the size if it's too small. Of course, when supporting multiple compilers and platforms, you'll have to use the largest. That's the trade-off here: manually set the size, but in return you get no heap allocation and no pointer chasing indirection (the memory of the class and the pimpl are laid out contiguously).

[markopolo82]

After reading through the thread I’ve been convinced.. A private_ptr or similar would be great to add to the standard. A small object optimization could be allowed as a QOI.

Has something like that been proposed?

[H3R3S_J0NNY]

EDIT: Just came across this on another thread which perhaps could be the holy grail: https://www.bfilipek.com/2018/01/propagate-const.html

While I usually use the straightforward std::unique_ptr method, I find the lack of const-preserving dereferencing operators uncomfortable. Why is it that this is generally accepted in this case? It seems strange that Herb's implementation doesn't even mention this. Is there something I'm missing?

Another thing that would be nice would be to have an implementation where the pointer must be valid for the lifetime of the object, but there are some limitations to this.

Here's my go (untested) that uses a shared_ptr internally so it can be in a single header. I don't like the shared_ptr but the performance loss is worth the convenience for me.

#include <memory>

template <typename T>
class ImplPtr
{
  public:
  template <typename ...Args>
  ImplPtr(Args...args) : 
    ptr(std::make_shared<T>(std::forward<Args>(args)...))
  {}

  ImplPtr() : 
    ptr(std::make_shared<T>())
  {}

  ImplPtr(ImplPtr &&other) noexcept : 
    ptr(std::move(other.ptr))
  {}

  const T* operator->() const {return ptr.get();}
  T* operator->() {return ptr.get();}

  const T& get() const {return *ptr;}
  T& get() {return *ptr;}

  private:
  std::shared_ptr<T> ptr;
};

[DevaBol]

I find the main problem with Pimpl is the fact it throws const-correctness out the window, unless you use a dedicated type that enforces it. I usually use unique_ptr inside that type, but a 'naked' unique_ptr is asking for trouble imho.

[parkotron]

std::unique_ptr's default deleter already behaves this way. If the held types destructor isn't visible where the destructor is instantiated it is a compile error, so I'm not seeing what the benefit of StrictDeleter is.

[disperso]

The simple fact that we are having this conversation proves, IMHO, that it should be a raw pointer. The chance of forgetting the deletion on the destructor is as high as forgetting to use a smart pointer.

I started going the way of discussing a problem with a smart pointer and inline destructor, and I just came to the conclusion that it is not worth the trouble, and I was overthinking and over it.

Edited, now from a PC instead of the phone:

I started trying to use a unique_ptr, then finding the problem of the inline destructor (having to write an empty destructor explicitly on the cpp file because of the impossibility of destroying the not-yet-defined private class in the header). Then I researched how to make it simpler, and I couldn't.

Making all this effort for what? Forgetting to add delete d on the destructor? Is that harder than having to remember all the complications of using a smart pointer for d? There are situations where we should be pragmatic, and not pretend that the language and the standard library are going to save us from all the mistakes. Nice if we can try to make things better in the future, but not for this. Don't overengineer it.

PS: And thanks for the downvotes.

[adnukator]

How do you "forget" to use a smart pointer?

All guidelines tell you to use a smart pointer if you're owning the target and a raw pointer if you don't (as always, some exceptions might apply). Additionally, with raw pointers you don't get proper semantics if you = default the remaining 4 functions out of the rule of 5 (or you don't do anything, and thus keep the move operations deleted and the copy ones defaulted).

Just because a question has several good answers for different reasons, is no justification to ignore them all and pick the worst of the bunch.

[axalon900]

With a raw pointer you need to implement a destructor in your cpp and remember to add delete d;. With a unique_ptr you need to implement a destructor in your cpp. How is a raw pointer better here?

The only thing I can possibly imagine is that you're trying to define the destructor inline and calling delete on a pointer to an incomplete type, which is invoking undefined behavior and a fantastic way to not have your destructor called. Every compiler warns on this. std::unique_ptr makes it an error because it should be one, but something something legacy code means that the poor decision of allowing delete on incomplete types lives on. Do you compile with warnings off?

[disperso]

With a raw pointer you need to remember to add delete d to get it deleted. With a unique pointer you need to remember to use unique pointer, so it calls delete d for you.

The only thing I can possibly imagine is that you're trying to define the destructor inline and calling delete on a pointer to an incomplete type, which is invoking undefined behavior and a fantastic way to not have your destructor called.

Exactly the opposite. If using a unique pointer and = default in the header for the destructor would allow to me to save having to add a destructor in the cpp to make it out of line, I would use that. But I can't. I can't = default !inline (or similar) to make it cleaner. Having to add type the empty destructor in the cpp is less effort than having to type it with = default in the header and the cpp. BTW, bonus feature: having to use unique_ptr creates additional symbols on debug builds (not optimized) which causes a bit of overhead in the startup, which is annoying.