Page 1 of 1

Putting SharedPtr<> child classes in their own header

Posted: Fri Jun 14, 2013 3:55 pm
by Mikachu
Hi,
In the process of pimpl'ing my library, I hit a wall with MeshPtr :
- I can't forward declare it, as it is passed by value
- If I want the complete type, I have to include "OgreMesh.h", which pulls many headers

It looks like a missed opportunity to reduce header dependencies :?

Would it be possible to put MeshPtr (and all other SharedPtr specialisations) in a separate header (which wouldn't depend on OgreMesh.h itself)?

Re: Putting SharedPtr<> child classes in their own header

Posted: Sat Jun 15, 2013 1:22 am
by Klaim
It should even be possible to declare all shared pointer types into one header using only forward declarations of necessary types without ever loading their headers. It's all pointers internally after all. I do this in my projects.

Re: Putting SharedPtr<> child classes in their own header

Posted: Sat Jun 15, 2013 3:50 pm
by Mikachu
Ok, I tried that (only for MeshPtr atm), but compile fails on the sharedptr destructor...
sharedptr.patch
(15.43 KiB) Downloaded 420 times
The line where compilation fails :

Code: Select all

case SPFM_DELETE_T:
				OGRE_DELETE_T(pRep, T, MEMCATEGORY_GENERAL);
				break;
It looks like Ogre's SharedPtr implementation requires knowing the complete type of the class it points to...

Does someone know if there's a way around that?

Re: Putting SharedPtr<> child classes in their own header

Posted: Sat Jun 15, 2013 10:42 pm
by Klaim
Wow I forgot about MeshPtr specializatoin... WOW I DIDn't KNEW THERE WAS VIRTUAL CALLS THERE !!! D:

From your description it looks like the deleter requires the type but it shouldn't.
Ah yes, maybe check if the headers types using the SharedPtr have a destructor into the header? If they do, move them in the cpp file instead. Then don't forget to include the necessary headers in the cpp file.

Re: Putting SharedPtr<> child classes in their own header

Posted: Tue Jun 18, 2013 8:29 am
by Mikachu
Klaim wrote:From your description it looks like the deleter requires the type but it shouldn't.
Ah yes, maybe check if the headers types using the SharedPtr have a destructor into the header? If they do, move them in the cpp file instead. Then don't forget to include the necessary headers in the cpp file.
The issue is that the problematic code comes from SharedPtr.h, and is inside a template, so it can't be put into a cpp...
This code is only triggered in the case where sharedPtr references a primitive type or external type, which is not the case of MeshPtr... so it would be possible, but not very elegant, to split SharedPtr in half... :shock:
I'm not sure it would be worth the cost... although I still believe it's pretty weird to have to include full type just to declare something with pointer semantics!

Re: Putting SharedPtr<> child classes in their own header

Posted: Fri Jun 21, 2013 9:59 am
by Klaim

Re: Putting SharedPtr<> child classes in their own header

Posted: Sat Jun 29, 2013 6:43 pm
by bstone
The delete operator absolutely needs to know the type as otherwise it can't call the proper destructor.

I think your best option would be taking advantage of precompiled headers. That will eliminate most overhead related to the need of including OgreMesh.h

Re: Putting SharedPtr<> child classes in their own header

Posted: Sun Jun 30, 2013 1:39 am
by Klaim
bstone wrote:The delete operator absolutely needs to know the type as otherwise it can't call the proper destructor.
It's correct but it don't mean that a smart pointer have to know the complete type it's pointing to in the user's header.
You can use a std::shared_ptr or std::unique_ptr this way perfectly legal, I use it often:

Code: Select all

//header

#include <memory>

class K; // not defined

class Foo
{
    std::shared_ptr<K> m_k1;
    std::unique_ptr<K> m_k2;
public:
    // because these following functions are only declared, 
    // they will not instantiate the constructor and destructor
    // of the member smart pointers if you include this header
    Foo();     
    ~Foo();
};

/////////////////////////////
// cpp file

#include "foo.hpp"
#include "k.hpp" // now we have the definition of K

Foo::Foo(){}      // smart pointer construction instantiated here
Foo::~Foo(){}    // smart pointer destruction instantiated here

This is because the point of instantiation is moved into the cpp file because the definition
of construction and destruction is there.

So there is no reason that Ogre::SharedPtr can't do the same.

Re: Putting SharedPtr<> child classes in their own header

Posted: Sun Jun 30, 2013 8:13 pm
by bstone
Yeah, and that's one of the reasons why std::unique_ptr<> is much more complicated than Ogre::SharedPtr.

Re: Putting SharedPtr<> child classes in their own header

Posted: Sun Jun 30, 2013 10:19 pm
by Klaim
bstone wrote:Yeah, and that's one of the reasons why std::unique_ptr<> is much more complicated than Ogre::SharedPtr.
I don't understand your point: I use this for non-pointers types too without any problem and their code is very simple,
It's only about instantiation of the destructor function of the smart pointer, so this technique should work with Ogre::SharedPtr too.

Re: Putting SharedPtr<> child classes in their own header

Posted: Thu Jul 04, 2013 1:01 pm
by Mikachu
Here's a post on stackoverflow that explains how std::shared_ptr (and boost::shared_ptr) doesn't need full type : http://stackoverflow.com/questions/5606 ... t-pointers

To sum up, there's a trick that involves storing the deleter when the shared pointer is bound to the raw pointer.
At this point, since it usually occurs just after a "new", the full type is known, thus the desctructor...
Of course, there's a small overhead involved with this operation.

I can try reproducing this behaviour with Ogre's SharedPtr, but my code will probably need some review, I'm no template guru :wink:

Re: Putting SharedPtr<> child classes in their own header

Posted: Tue Jul 09, 2013 10:34 am
by Mikachu
I managed to change Ogre's SharedPtr implementation so that it doesn't need full type at declaration time... :D
Instead, full type is only needed at binding time, when full type is already known anyway.

I'll further test it, then will issue a pull request.

It should help reducing inter-header dependency :) (a static analysis tool would be helpful to tell which header inclusion became useless)

Re: Putting SharedPtr<> child classes in their own header

Posted: Fri Jul 12, 2013 10:28 am
by Mikachu
Ok, my patch to SharedPtr has been merged into branch v1-9.
Now, I just need to remove un-needed header include...

Does someone have a tool to suggest?
Such a tool should be able to take into account forward definitions, and ignore PCH header...

Re: Putting SharedPtr<> child classes in their own header

Posted: Mon Jan 13, 2014 2:31 pm
by bstone
Klaim wrote:
bstone wrote:Yeah, and that's one of the reasons why std::unique_ptr<> is much more complicated than Ogre::SharedPtr.
I don't understand your point: I use this for non-pointers types too without any problem and their code is very simple,
It's only about instantiation of the destructor function of the smart pointer, so this technique should work with Ogre::SharedPtr too.
Sorry, couldn't reply any earlier but Mikachu already explained that. And great job Mikachu, thanks for healing Ogre::SharedPtr! Can't advise on the static analysis tool though.