Putting SharedPtr<> child classes in their own header

What it says on the tin: a place to discuss proposed new features.
Post Reply
User avatar
Mikachu
Gnoll
Posts: 603
Joined: Thu Jul 28, 2005 4:11 pm
Location: Nice, France

Putting SharedPtr<> child classes in their own header

Post by Mikachu » Fri Jun 14, 2013 3:55 pm

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)?
0 x
OgreProcedural - Procedural Geometry for Ogre3D

User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
Contact:

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

Post by Klaim » Sat Jun 15, 2013 1:22 am

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.
0 x

User avatar
Mikachu
Gnoll
Posts: 603
Joined: Thu Jul 28, 2005 4:11 pm
Location: Nice, France

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

Post by Mikachu » Sat Jun 15, 2013 3:50 pm

Ok, I tried that (only for MeshPtr atm), but compile fails on the sharedptr destructor...
sharedptr.patch
(15.43 KiB) Downloaded 101 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?
0 x
OgreProcedural - Procedural Geometry for Ogre3D

User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
Contact:

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

Post by Klaim » Sat Jun 15, 2013 10:42 pm

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.
0 x

User avatar
Mikachu
Gnoll
Posts: 603
Joined: Thu Jul 28, 2005 4:11 pm
Location: Nice, France

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

Post by Mikachu » Tue Jun 18, 2013 8:29 am

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!
0 x
OgreProcedural - Procedural Geometry for Ogre3D

User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
Contact:

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

Post by Klaim » Fri Jun 21, 2013 9:59 am

0 x

bstone
OGRE Expert User
OGRE Expert User
Posts: 1920
Joined: Sun Feb 19, 2012 9:24 pm
Location: Russia

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

Post by bstone » Sat Jun 29, 2013 6:43 pm

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
0 x

User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
Contact:

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

Post by Klaim » Sun Jun 30, 2013 1:39 am

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.
0 x

bstone
OGRE Expert User
OGRE Expert User
Posts: 1920
Joined: Sun Feb 19, 2012 9:24 pm
Location: Russia

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

Post by bstone » Sun Jun 30, 2013 8:13 pm

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

User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
Contact:

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

Post by Klaim » Sun Jun 30, 2013 10:19 pm

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.
0 x

User avatar
Mikachu
Gnoll
Posts: 603
Joined: Thu Jul 28, 2005 4:11 pm
Location: Nice, France

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

Post by Mikachu » Thu Jul 04, 2013 1:01 pm

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:
0 x
OgreProcedural - Procedural Geometry for Ogre3D

User avatar
Mikachu
Gnoll
Posts: 603
Joined: Thu Jul 28, 2005 4:11 pm
Location: Nice, France

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

Post by Mikachu » Tue Jul 09, 2013 10:34 am

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)
0 x
OgreProcedural - Procedural Geometry for Ogre3D

User avatar
Mikachu
Gnoll
Posts: 603
Joined: Thu Jul 28, 2005 4:11 pm
Location: Nice, France

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

Post by Mikachu » Fri Jul 12, 2013 10:28 am

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...
0 x
OgreProcedural - Procedural Geometry for Ogre3D

bstone
OGRE Expert User
OGRE Expert User
Posts: 1920
Joined: Sun Feb 19, 2012 9:24 pm
Location: Russia

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

Post by bstone » Mon Jan 13, 2014 2:31 pm

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.
0 x

Post Reply