1

foo.h

#include"fooImpl.h"

    class foo
    { 
        private:
            class fooImpl; 
            std::unique_ptr <fooImpl> foo_imple;
        public:
            void bar();  
    };

fooImpl.h

#include "foo.h"

    class foo::fooImpl 
    {
        public: 
            void api();  
    };

foo.cpp

#include "foo.h"

void foo::bar()
{
    foo_imple->api();
}

fooImpl.cpp

#include"foo.h"
#include"fooImpl.h"

void foo::fooImpl::api()       
{   
    cout << "bar"; 
}

Getting the following error:

error: invalid use of incomplete type 'class foo::fooImpl'
foo_imple->bar();

note: forward declaration of 'class foo::fooImpl'
class fooImpl;

Not able to find out whats wrong with this pimpl implementation. Error looks like due to some declaration missing.

SHR
  • 7,940
  • 9
  • 38
  • 57
Shibir Basak
  • 101
  • 7
  • Possible duplicate of [How are circular #includes resolved?](https://stackoverflow.com/questions/3127171/how-are-circular-includes-resolved) – 1201ProgramAlarm Aug 12 '18 at 15:51

2 Answers2

4

This is more typical:

foo.h

class foo
{ 
    private:
        class fooImpl;
        std::unique_ptr <fooImpl> foo_imple;

    public:
        void bar();
        // Non-inline destructor so we don't need a definition for fooImpl
        ~foo();
};

foo.cpp

#include "foo.h"

class foo::fooImpl
{
    public:
        void api()
        {
            cout << "bar";
        }
};

void foo::bar()
{
    foo_imple->api();
}

foo::~foo() = default;

Notice that now you don't need to expose anything about the fooImpl to users of foo - the class is completely hidden. You also avoid having additional header and cpp files if fooImpl is only used in your foo.cpp source file, otherwise you could put it and its method definitions inline in a fooImpl.h that only needs to be included in foo.cpp.

Chris Hunt
  • 3,840
  • 3
  • 30
  • 46
  • 3
    Better: `foo::~foo() = default;` (in the same location, not inside the class) But yes, controlling the destructor definition is very necessary as noted here: https://stackoverflow.com/q/8595471/103167 – Ben Voigt Aug 12 '18 at 18:24
  • @ChrisHunt In my case, I must move the impl to a separate file. This is because of ownership/modularity constraints of the project. – Shibir Basak Aug 13 '18 at 06:07
3

In order to call foo_imple->bar(); class declaration from fooImpl.h should be available. While for foo class declaration there is no need to include fooImpl.h

user7860670
  • 35,849
  • 4
  • 58
  • 84
  • So, what edits are you suggesting? Adding fooImpl.h to foo.cpp and removal of #include"fooImpl.h" from foo.h? – Shibir Basak Aug 12 '18 at 16:02
  • @ShibirBasak Exactly. – user7860670 Aug 12 '18 at 16:03
  • Worked! Thanks. – Shibir Basak Aug 12 '18 at 16:29
  • 2
    This fix *doesn't* work if `fooImpl` has any non-trivial destructor. See @ChrisHunt's answer and also https://stackoverflow.com/q/8595471/103167 for the fix. – Ben Voigt Aug 12 '18 at 18:26
  • Getting this, when using assert in api() --> /usr/include/c++/6.2.0/bits/unique_ptr.h:74:22: error: invalid application of 'sizeof' to incomplete type 'foo::fooImpl' static_assert(sizeof(_Tp)>0, ^ – Shibir Basak Aug 13 '18 at 07:15
  • @ShibirBasak I don't see any `assert` in `api()`, so I guess you are asking about some code that not present in the question, right? Putting assert in the `api()` as it written here should not cause any problems because class `fooImpl` is complete at that point. – user7860670 Aug 13 '18 at 08:37
  • @BenVoigt This fix *does* work regardless of whether `fooImpl` has trivial or non-trivial destructor since this class will be complete in both translation units mentioned in the question. The link you posted is indeed helpful, however even with an explicit destructor of `foo` the includes should be rearranged as written in my answer. – user7860670 Aug 13 '18 at 08:53
  • @VTT. Even without any assert. This code gives error : Complete error log here: ...... c++/6.2.0/bits/unique_ptr.h: In instantiation of 'void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = foo::fooImpl]': ... required from 'std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = foo::fooImpl; _Dp = std::default_delete]' ... error: invalid application of 'sizeof' to incomplete type 'foo::fooImpl' static_assert(sizeof(_Tp)>0, – Shibir Basak Aug 13 '18 at 11:53
  • @ShibirBasak Are you sure that error comes from `fooImpl.cpp` and not from other translation unit? Note that without explicit destructor for `foo` class `fooImpl` definition must be available in every translation unit that uses `foo`. – user7860670 Aug 13 '18 at 18:46
  • @VTT An object of foo was created by another translation unit. I added inline constructor and destructor in foo.hpp. This error was resolved when I moved them to foo.cpp. Thanks to Modern Effective C++ by Scott Meyers : Item 22: When using the Pimpl Idiom, define special member functions in the implementation file. – Shibir Basak Aug 14 '18 at 06:51