21

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.

Ben Voigt
  • 260,885
  • 36
  • 380
  • 671
  • 2
    `Paging` Dr. @HerbSutter – sehe Dec 22 '11 at 23:59
  • @Ben Voigt: I did search for and tag several other questions with gotw. Did you find some that I missed? – Adrian McCarthy Dec 27 '11 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. – Adrian McCarthy Dec 28 '11 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" :) – John Oct 11 '12 at 22:02

2 Answers2

10

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.

MSN
  • 49,698
  • 7
  • 70
  • 100
  • 3
    `widget::~widget()` needs to call the destructors for all subobjects. Where is `pimpl::~pimpl()` instantiated, so that `widget::~widget()` can call it? – Ben Voigt Dec 21 '11 at 20:02
  • @BenVoigt: It's instantiated at the point it's used, i.e. the definition of `widget::~widget()` in the `.cpp` file. – Mike Seymour Dec 21 '11 at 20:36
  • 4
    @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. – Ben Voigt Dec 21 '11 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`. – MSN Dec 21 '11 at 22:51
7

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?1), 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.


1 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.
sehe
  • 328,274
  • 43
  • 416
  • 565
  • Added some of the more obvious arguments as to what the pimpl wrapper solves, besides the focus of the OP's question. – sehe Dec 21 '11 at 20:40
  • 4
    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. – Ben Voigt Dec 21 '11 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. – sehe Dec 21 '11 at 21:21
  • 1
    sehe, are you copying from above or below the subheading marked "Solution" in GotW #101? – Ben Voigt Dec 21 '11 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 – sehe Dec 21 '11 at 21:23
  • 4
    To conclude: Herb says the wrapper "eliminates some pieces of boilerplate". It seems like the code doesn't work if those are eliminated. – Ben Voigt Dec 21 '11 at 21:24
  • Did you have a chance to look at this any further? – Ben Voigt Dec 22 '11 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, which causes a missing external problem at link time. – Adrian McCarthy Dec 22 '11 at 23:20
  • @AdrianMcCarthy: I think you got the destructor name wrong there. I'm guessing you meant `pimpl::~pimpl()`, which I mentioned in the question as being uninstantiated. – Ben Voigt Dec 22 '11 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. – sehe Dec 22 '11 at 23:58