5

I apologize for the large amount of code required to demonstrate the issue. I am having a problem using the pimpl idiom with std::unique_ptr. Specifically the problem seems to occur when one class (which has pimpl'ed implementation) is used as member data in another composite class with pimpl'ed implementation.

Most of the answers I've been able to find deal with a lack of explicit destructor declaration, but as you can see here, I have declared and defined the destructors.

What is wrong with this code, and can it be modified to compile without changing the design?

Note: the error seems to occur in the definition of SomeComposite::getValue() and that the compiler cannot see the error until compile time. The error is encountered in memory.h and the message is Invalid application of 'sizeof' to an incomplete type 'pimplproblem::SomeInt::impl'.

SomeInt.h

#pragma once
#include <iostream>
#include <memory>

namespace pimplproblem
{
    class SomeInt
    {

    public:
        explicit SomeInt( int value );
        SomeInt( const SomeInt& other ); // copy
        SomeInt( SomeInt&& other ) = default; // move
        virtual ~SomeInt();
        SomeInt& operator=( const SomeInt& other ); // assign
        SomeInt& operator=( SomeInt&& other ) = default; // move assign
        int getValue() const;

    private:
        class impl;
        std::unique_ptr<impl> myImpl;
    };
}

SomeInt.cpp

#include "SomeInt.h"

namespace pimplproblem
{
    class SomeInt::impl
    {
    public:
        impl( int value )
        :myValue( value )
        {}

        int getValue() const
        {
            return myValue;
        }
    private:
        int myValue;
    };

    SomeInt::SomeInt( int value )
    :myImpl( new impl( value ) )
    {}

    SomeInt::SomeInt( const SomeInt& other )
    :myImpl( new impl( other.getValue() ) )
    {}

    SomeInt::~SomeInt()
    {}

    SomeInt& SomeInt::operator=( const SomeInt& other )
    {
        myImpl = std::unique_ptr<impl>( new impl( other.getValue() ) );
        return *this;
    }

    int SomeInt::getValue() const
    {
        return myImpl->getValue();
    }
}

SomeComposite.h

#pragma once
#include <iostream>
#include <memory>
#include "SomeInt.h"

namespace pimplproblem
{
    class SomeComposite
    {   

    public:
        explicit SomeComposite( const SomeInt& value );
        SomeComposite( const SomeComposite& other ); // copy
        SomeComposite( SomeComposite&& other ) = default; // move
        virtual ~SomeComposite();
        SomeComposite& operator=( const SomeComposite& other ); // assign
        SomeComposite& operator=( SomeComposite&& other ) = default; // move assign
        SomeInt getValue() const;

    private:
        class impl;
        std::unique_ptr<impl> myImpl;
    };
}

SomeComposite.cpp

#include "SomeComposite.h"

namespace pimplproblem
{
    class SomeComposite::impl
    {
    public:
        impl( const SomeInt& value )
        :myValue( value )
        {}

        SomeInt getValue() const
        {
            return myValue;
        }
    private:
        SomeInt myValue;
    };

    SomeComposite::SomeComposite( const SomeInt& value )
    :myImpl( new impl( value ) )
    {}

    SomeComposite::SomeComposite( const SomeComposite& other )
    :myImpl( new impl( other.getValue() ) )
    {}

    SomeComposite::~SomeComposite()
    {}

    SomeComposite& SomeComposite::operator=( const SomeComposite& other )
    {
        myImpl = std::unique_ptr<impl>( new impl( other.getValue() ) );
        return *this;
    }

    SomeInt SomeComposite::getValue() const
    {
        return myImpl->getValue();
    }
} 
Cœur
  • 37,241
  • 25
  • 195
  • 267
Matthew James Briggs
  • 2,085
  • 2
  • 28
  • 58

1 Answers1

7

You can't use defaulted constructors and assignment operators (such as SomeInt( SomeInt&& other ) = default;) declared in header file with Pimpl classes, because the default implementations are inline, and at the point of declaration SomeInt's declaration SomeInt::impl is incomplete, so unique_ptr complains. You have to declare and define out of line (that is, in implementation file) all special member functions yourself.

That is, change SomeInt and SomeComposite declarations as follows:

// SomeInt.h
SomeInt( SomeInt&& other ); // move
SomeInt& operator=( SomeInt&& other ); // move assign

// SomeInt.cpp
// after definition of SomeInt::impl
SomeInt::SomeInt( SomeInt&& other ) = default;
SomeInt& operator=( SomeInt&& other ) = default;

Another option is to create your own Pimpl pointer, as suggested in this answer.

Community
  • 1
  • 1
Anton Savin
  • 40,838
  • 8
  • 54
  • 90
  • 1
    This answer http://stackoverflow.com/a/11212403/2779792 led me astray. It seems like the answer there is dead wrong since it shows a completed declaration of the impl class in the header file(?), which defeats the purpose of pimpl. Then the answer goes on use default moves. Maybe I didn't understand the Q/A because my situation is apparently different. – Matthew James Briggs Feb 28 '15 at 21:43
  • 1
    @MatthewJamesBriggs the highest-rated comment to that question suggests to read [GOTW 100](http://herbsutter.com/gotw/_100/) which is much more useful than the answer. I also updated my answer with more concrete instructions what to do. – Anton Savin Feb 28 '15 at 21:58
  • @MatthewJamesBriggs: That answer is not wrong, but it does things differently, and does not provide a compilation firewall at all. Notice that in that answer the body of `impl` is in the header file, not in the implementation file as yours is. It also addresses the problems which arise when the `impl` definition is moved. – Ben Voigt Feb 28 '15 at 23:21
  • As a note, you can still get _part_ of the benefit of defaulted special member functions when using PIMPL, since you just can't declare them in the _header_. It's perfectly valid to have, e.g., `SomeInt(SomeInt&& other);` in the header and `SomeInt(SomeInt&& other) = default;` in the source file, as long as the `= default` definition comes after `SomeInt::impl`'s definition. The only downside is that it isn't trivial, but it wasn't going to be trivial anyways, so that's not a big loss. – Justin Time - Reinstate Monica Aug 11 '23 at 03:02