Putting SharedPtr<> child classes in their own header

[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)?

[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.

[Mikachu]

Ok, I tried that (only for MeshPtr atm), but compile fails on the sharedptr destructor...

sharedptr.patch

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?

[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.

[Mikachu]

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...
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!

[Klaim]

A related post: http://www.ogre3d.org/forums/viewtopic. ... ad#p491761

[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

[Klaim]

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.

[bstone]

Yeah, and that's one of the reasons why std::unique_ptr<> is much more complicated than Ogre::SharedPtr.

[Klaim]

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.

[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

[Mikachu]

I managed to change Ogre's SharedPtr implementation so that it doesn't need full type at declaration time...
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)

[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...

[bstone]

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.