views:

222

answers:

7

I have a class CContainer that has some members CMemberX, CMemberY, which are independent of each other and other CClientA, CClientB classes that use CContainer.

#include "MemberX.h"
#include "MemberY.h"

class CContainer
{
public:
    CMemberX & GetX() const { return m_x; }
    CMemberY & GetY() const { return m_y; }

private:
    CMemberX m_x;
    CMemberY m_y;
};

I want to avoid having to recompile all CClient classes when modifying one of the CMember classes using forward declarations and dynamic allocation of m_x and m_y.

Initially, I made the members pointers:

// Container.h
class CMemberX;
class CMemberY;

class CContainer
{
public:
    CContainer();
    ~CContainer();

    CMemberX & GetX() const { ASSERT(m_pX != NULL); return *m_pX; }
    CMemberY & GetY() const { ASSERT(m_pY != NULL); return *m_pY; }

private:
    CMemberX* m_pX;
    CMemberY* m_pY;
};

// Container.cpp
#include "Container.h"
#include "MemberX.h"
#include "MemberY.h"

// Allocate members on heap
CContainer::CContainer() : m_pX(new CMemberX()), m_pY(new CMemberY()) {}
CContainer::~CContainer() { delete m_pX; delete m_pY; }

Then I thought, that I could as well use references instead of pointers, so it looks more like the original code:

// Container.h
class CMemberX;
class CMemberY;

class CContainer
{
public:
    CContainer();
    ~CContainer();

    CMemberX & GetX() const { return m_x; }
    CMemberY & GetY() const { return m_y; }

private:
    CMemberX & m_x;
    CMemberY & m_y;
};

// Container.cpp
#include "Container.h"
#include "MemberX.h"
#include "MemberY.h"

// Allocate members on heap
CContainer::CContainer() : m_x(*new CMemberX()), m_y(*new CMemberY()) {}
CContainer::~CContainer() { delete &m_x; delete &m_y; }

What I don't like about the pointer members is that it looks like the pointers could be NULL or the objects be replaced at runtime, which is not the case.

What I don't like about the references is that the code in the CTor and DTor looks a bit hacky.

Which approach is preferable? Is there a better solution?

Note regarding copying/assigning: Instances of the CContainer class will not under any circumstances be copied or assigned to each other.

+2  A: 

There have been a lot of questions here about the desirability of using references as members (for example http://stackoverflow.com/questions/892133/should-i-prefer-pointers-or-references-in-member-data), and it seems to me that the majority opinion (which also happens to be mine) is - don't. If you don't want the pointers to be changed make them const - I can't see how, given your code, they can possibly be NULL.

anon
Indeed, references as members is a no-no!
haavee
I disagree, but then I disagreed in more detail on several of the other similar questions. Neil, it might be a good idea to add links to the similar questions to your answer.
Len Holgate
As I write, the highest-voted answer on that link is in favour of reference members, as is the third highest. So I don't know how a claim of "overwhelming opinion" can be at all justified. The accepted answer lists three "example problems", two of which are active advantages in this questioner's use-case. The other says "you can't easily change a reference later to use a pointer", which hardly helps this questioner, since he already can't easily change his object to a pointer. Even if reference members are a bad idea, -1 for appeal to false authority.
Steve Jessop
@Steve There are a lot of other questions on this topic, I suggest you take a look.
anon
I disagree. And in the other question you are the only one that makes that particular point so it is not universal. The only piece of code that actually works above is the first one. The other two have serious flaws. Also in this case he wants to use references/pointers to avoid problems with re-compilation when the objects are natural members of the class.
Martin York
I've modified my answer - not that I expect this will make anyone happy.
anon
Cheered me up enough to remove my -1, as it happens ;-)
Steve Jessop
+1  A: 

Hi,

Dynamic allocation does nothing for you in respect of what you want: not having to recompile CClient and CContainer.

The only allowed use when using forward declarations is declaring pointer(s) to the forward-declared type.

As soon as you use a method or a member of the forward-declared type it will not compile: the compiler MUST know the full type of whatever it is using.

In short: either you never have to recompile [you are only declaring pointers to the forward declared type] OR you always have to recompile, in case you actually DO use CContainer.

haavee
All it prevents is the need to recompile users of CClient and CContainer.
e8johan
It does help, when I only have to include the headers in the .cpp file instead of the .h file.
mxp
Then you should store pointers to MemberX and MemberY. That's the most expressive way. Whatever you wish to return (references or pointers) to your CClients is up to your personal taste.
haavee
+5  A: 

I think that's what the const variables are for:

CMember * const m_x;

Cannot change m_x after initialization...

Arkadiy
Recomending using pointers when they are actually members is not a good choice. Anyway without adding explicit copy constructors etc. the class needs smart pointers not RAW pointers.
Martin York
Damn, you are right! A better thing to do is const auto_ptr<CMember> m_x;
Arkadiy
A: 

In the second block of code in your question you have private member pointers which are initialised and destroyed along with the parent class. This information should be enough to the reader of your code to realise what is going on.

In addition you could declare the pointers const: CMember* const m_pX; to indicate that they cannot be changed after initialisation. Now the compiler will catch accidental changes.

quamrana
The real code is not as clean as the sample above and would require the reader of the code to dive into the implementation file instead of just checking the header.
mxp
+3  A: 

I think it's a little surprising to use a reference when there are ownership semantics. Doesn't necessarily make it a bad idea, all things considered, but it does weigh against.

I think I've only ever used references as members in cases where both:

  • an object is provided to the constructor, which is required to outlive this object.
  • assignment is forbidden anyway.

So for example, injected dependencies such as factory or service objects could be suitable. As against that, in C++ you'd often prefer to inject dependencies with template parameters rather than objects, so the issue might not arise.

I also find that the longer I use C++, the more I want types to be Assignable unless there's a really good reason not to be. The usual trick for reducing compile-time dependencies in the way you want is "Pimpl", not "Rimpl", for a reason. By switching from an object member to a reference member, you are making your class non-default-copyable, where previously perhaps it was copyable. This implementation detail shouldn't constrain the class's interface. With Pimpl you can cleanly implement assignment and swap. With these references you would have to assign or swap both members. If the second swap fails, you've lost the strong exception guarantee: although if your CMemberX and CMemberY classes have no-fail assignment and swap, this doesn't matter.

So I don't think I like the reference in this case, but then I haven't seen the rest of your code. Maybe there's some reason why none of the concerns about assignment apply - for instance if CContainer is itself a RAII class, then usually the only lifecycle operations it should support are construction and destruction.

Steve Jessop
A: 

You are not really buying yourself anything.
(Slightly shorter compile time in limited situations).

But you are heaping a whole lot of other code that needs to be maintained onto your plate.

If these objects are natural members then leave them as members.
By creating them on the stack and storing them as pointers or references you have to ask a whole bunch of sticky questions that need code to answer them.

  • What happens when you copy construct the object.
    • I would normally expect the obejct and all its members to be copied.
      Using pointers or references you are going to have to do extra work to replicate this functionality that is already provided.
  • What happens when you assign the objects.
    • References will not work (though you get around this by using boost references).
      But you still have the problem with expecting the members to be copied.
  • What happens when you delete the object.
    • If you have implemented all the above correctly to make copies fine.
      Otherwise you need to start thinking about shared pointers to implement the functionality you need.

As it stands versions 2 and 3 (of the code in the question) are seriously flawed and the only version that actually works is 1.

In my mind the simple fact that maintenance costs will be so much lower with version 1, that recommending either of version 2 or 3 is counter productive. The extra time to compile one more class when a member is changed is relatively small in comparison to the complexity you are adding to the code.

Also you mention in somebody else s comment that the code is not quite as clean as that described above. This just emphasizes my point that this is bad optimization that will make it hard to get the class working correctly and keep it maintained in that state.

Martin York
Regarding compile time: In my case, it is significant. The whole project has some issues with dependencies, because in some way nearly every class accesses that CContainer. Compiling after changing one of the CMember classes nearly equalled a full rebuild. I would have made it different but this is how it is.
mxp
+1  A: 

Steve Jessop already mentioned the pImpl idiom in passing, but I think you should check this out if you haven't already come across it: Compilation Firewalls

James Hopkin
I see the point of you both. But doing it "the way it's meant to be done" i.e. with interface methods that call implementation methods, appears to be a lot of effort plus increase of complexity without additional gain to me.
mxp
In cases where the contained objects are complex enough to cause significant compile time increases, the pImpl idiom is worth it. Any other classes that contain the same objects will automatically benefit. If they're not complex, perhaps there are other problems causing disproportionate compile times.
James Hopkin