Does the GotW #101 "solution" actually solve anything?

Asked 21/12, 2011 at 19:50 Answered 21/12, 2011 at 20:21

Solved c++c++11 pimpl-idiom incomplete-type gotw

First read Herb's Sutters GotW posts concerning pimpl in C++11:

I'm having some trouble understanding the solution proposed in GotW #101. As far as I can understand, all the problems laboriously solved in GotW #100 are back with a vengeance:

The pimpl members are out-of-line templates, and the definitions are not visible at the point of use (in class widget's class definition and implicitly generated special member functions of widget). There aren't any explicit instantiations either. This will cause unresolved external errors during linking.
widget::impl is still incomplete at the point where pimpl<widget::impl>::~pimpl() is ~~instantiated~~ defined (I don't think it actually IS instantiated at all, just referenced). So std::unique_ptr<widget::impl>::~unique_ptr() calls delete on a pointer to incomplete type, which produces undefined behavior if widget::impl has a non-trivial destructor.

Please explain what forces the compiler to generate the special members in a context where widget::impl is complete. Because I can't see how this works.

If GotW #101 still requires explicit definition of widget::~widget() in the implementation file, where widget::impl is complete, then please explain the "More Robust" comment (which @sehe quoted in his answer).

I'm looking at the core claim of GotW #101 that the wrapper "eliminates some pieces of boilerplate", which seems to me (based on the remainder of the paragraph) to mean the widget::~widget() declaration and definition. So please don't rely on that in your answer, in GotW #101, that's gone!

Herb, if you stop by, please let me know if it would be ok to cut+paste the solution code here for reference.

Cruck answered 21/12, 2011 at 19:50 Comment(4)

Paging Dr. @HerbSutter – Chickabiddy 22/12, 2011 at 23:59

@Ben Voigt: I did search for and tag several other questions with gotw. Did you find some that I missed? – Bitthia 27/12, 2011 at 20:28

@Ben Voigt: Must be a propagation delay in Stack Exchange then. I added the tag to several other questions immediately after creating it for this one. I've found one more question since then and tagged it as well. – Bitthia 28/12, 2011 at 16:54

I have just read article, and I have same difficulties with understanding how it is better - because obviously it is not, or code is incomplete. I get there by google "gotw #101 does not work" :) – Gothicize 11/10, 2012 at 22:2

You are correct; the example seems to be missing an explicit template instantiation. When I try to run the example with a constructor and destructor for widget::impl on MSVC 2010 SP1, I get a linker error for pimpl<widget::impl>::pimpl() and pimpl<widget::impl>::~pimpl(). When I add template class pimpl<widget::impl>;, it links fine.

In other words, GotW #101 eliminates all boilerplate from GotW #100, but you need to add an explicit instantiation of the pimpl<...> template with the implementation of the pimpl impl. At least with #101 the boiler plate you need is straightforward.

Forseti answered 21/12, 2011 at 19:53 Comment(4)

widget::~widget() needs to call the destructors for all subobjects. Where is pimpl<widget::impl>::~pimpl() instantiated, so that widget::~widget() can call it? – Cruck 21/12, 2011 at 20:2

@BenVoigt: It's instantiated at the point it's used, i.e. the definition of widget::~widget() in the .cpp file. – Cogitable 21/12, 2011 at 20:36

@MikeSeymour: The GotW #100 version defines widget::~widget() in the .cpp file. GotW #101 doesn't, it says it "eliminates some pieces of boilerplate", and specifically mentions that destructor. – Cruck 21/12, 2011 at 21:25

@BenVoigt, you were right. I tried compiling it to make sure, and the implicit instantiation of pimpl does not include instantiating its constructor or destructor when implementing widget::impl. – Forseti 21/12, 2011 at 22:51

I think the confusion is this: the pimpl wrapper may be a template, the widget class isn't:

`demo.h`

#include "pimpl_h.h"

// in header file
class widget {
public:
    widget();
    ~widget();
private:
    class impl;
    pimpl<impl> pimpl_;
};

demo.cpp

#include "demo.h"
#include "pimpl_impl.h"

// in implementation file
class widget::impl {
    // :::
};

widget::widget() : pimpl_() { }
widget::~widget() { } // or =default

As you can see, nobody will ever see a 'templated' constructor for the widget class. There will be only one definition of it, and no need for 'explicit instantiation'.

Conversely, the ~pimpl<> destructor is only ever instantiated from the single point of definition of the ~widget() destructor. At that point the impl class is complete, by definition.

There are no linkage errors and no ODR/UB violations.

Bonus/extra benefits

As Herb himself aptly explains in his post (Why is this an improvement over the hand-rolled Pimpl Idiom?¹), there are many more advantages to using this pimpl wrapper, that stem from reusing the implementation guts:

use a template to guard against walking into pitfalls unnecessarily
you get the variadic, perfect forwarding constructors with C++0x/C++11, no need to dream that templated constructor template with rvalue-reffed variadic argument list forwarding the argument pack to the wrapped classes' constructor perfectly etc. etc.

In short: DRY and convenience.

_{¹ to quote (emphasis mine):}

^{First, the code is simpler because it eliminates some pieces of boilerplate: In the hand-rolled version, you also have to declare the constructor and write its body in the implementation file and explicitly allocate the impl object. You also have to remember to declare the destructor and write its body in the implementation file, for obscure language reasons explained in #100.}

^{Second, the code is more robust: In the hand-rolled version, if you forget to write the out-of-line destructor, the Pimpl’d class will compile in isolation and appear to be in a check-in-able state, but will fail to compile when used by a caller that tries to destroy an object and encounters a helpful “cannot generate destructor because impl is, uh, you know, incomplete” error that leaves the author of the calling code scratching his head as he walks over to your office to ream you out for checking in something broken.}

Chickabiddy answered 21/12, 2011 at 20:21 Comment(10)

Added some of the more obvious arguments as to what the pimpl wrapper solves, besides the focus of the OP's question. – Chickabiddy 21/12, 2011 at 20:40

But Herb's "solution" doesn't have a non-default widget::~widget or out-of-line definition. He said that "in the hand-rolled version, you have to remember to declare the destructor and write its body in the implementation file", you're now saying you still need to do that for the wrapper. Which contradicts the claims made in the robustness bullet. – Cruck 21/12, 2011 at 21:16

I have just copied that out-of-line destructor out of his code? Now, if you argue that defaulting it, might lead to the same problem, I'd have to refer to the standard, but I strongly suggest that the compiler should emit that defaulted destructor at the point where ~widget() = default; is declared, i.e. at the time where impl is a complete type. – Chickabiddy 21/12, 2011 at 21:21

sehe, are you copying from above or below the subheading marked "Solution" in GotW #101? – Cruck 21/12, 2011 at 21:22

Hmm. Hold on. I have had the code snippets mixed up! You might be on to something there. Will be back later – Chickabiddy 21/12, 2011 at 21:23

To conclude: Herb says the wrapper "eliminates some pieces of boilerplate". It seems like the code doesn't work if those are eliminated. – Cruck 21/12, 2011 at 21:24

Did you have a chance to look at this any further? – Cruck 22/12, 2011 at 21:59

The code you quoted is the recap of the hand-rolled solution to #100, not the answer for #101. #101 supposedly lets you leave out the explicit, out-of-line destructor for widget. But if you leave it out, the compiler doesn't instantiante pimp<impl>::~impl, which causes a missing external problem at link time. – Bitthia 22/12, 2011 at 23:20

@AdrianMcCarthy: I think you got the destructor name wrong there. I'm guessing you meant pimpl<widget::impl>::~pimpl(), which I mentioned in the question as being uninstantiated. – Cruck 22/12, 2011 at 23:44

@BenVoigt: sadly, no, swamped in work and music performances (that time of the year). I'm going to agree with your conclusion for now, surprising as it seems for Herb to have missed that. I can't convince myself that the explicit destructor can be done without as things stand. I'm still wanting to check the actual emitted code to see whether there is some hidden clue. Which would then lead to a very thorough search of the specs to see why it would work. – Chickabiddy 22/12, 2011 at 23:58

Hot tags

Godot Unity Godot Help Programming Godot 4.X GUI GDScript 3D 2D Physics CSharp Godot 3.X VR XR Projects C++

demo.h

demo.cpp

Bonus/extra benefits

Recommended topics

Hot tags

`demo.h`