3

I'm woring against a model which consists of a number of different types (Properties, Parent, Child, etc). Each type is associated with a set of functions from a c api. For example:

Type "Properties":
  char* getName(PropertiesHandle);
  char* getDescription(PropertiesHandle);

Type "Parent"
  PropertiesHandle getProperties(ParentHandle);
  ChildHanlde getFirstChild(ParentHandle);

Type "Child"
  PropertiesHandle getProperties(ChildHandle);
  ParentHanlde getParent(ChildHandle);
  ChildHandle getNextChild(ChildHandle);

I have in turn created a set of C++ interfaces for this c api library, as follows:

class IProperties
{
public:
  virtual std::string getName() = 0;
  virtual std::string getDescription() = 0;
};

class IParent
{
public:
  virtual std::shared_ptr<IProperties> getProperties() = 0;
  virtual std::shared_ptr<IChild> getFirstChild() = 0;
};

class IChild
{
public:
  virtual std::shared_ptr<IProperties> getProperties() = 0;
  virtual std::shared_ptr<IParent> getParent() = 0;
  virtual std::shared_ptr<IChild> getNextChild() = 0;
};

I then implement these interfaces via the classes Properties, Parent and Child.

So a Child instance will take its specific ChildHandle via its constructor and its getParent function will look something like this:

std::shared_ptr<IParent> getParent()
{
    // get the parent handle and wrap it in a Parent object
    return std::shared_ptr<IParent>(new Parent(_c_api->getParent(_handle)));
}

Is it reasonable for me to return a shared_ptr here in your opinion. I cant use std::unique_ptr since Google Mock requires parameters and return values of mocked methods to be copyable. I'm mocking these interfaces in my tests via Google Mock.

I'm thinking also about how things might get optimized in the future which might present the possibly of circular references. This could be caused if caching is used to avoid multiple calls to the C api (for example, no need for a child to establish its parent more than once) combined with say the Child constructor taking its Parent. This would then require the use of weak_ptrs which would change the interfaces and a lot of my code...

Baz
  • 12,713
  • 38
  • 145
  • 268
  • 1
    This seams reasonable. From the code above your objects appear to be facades for the underlying object model, and `getParent()` is handing off new facade instance. The one concern I would have is that the lifetime of `Parent` with respect to the underlying object. Ideally, the underlying object would be reference counted as well, and creating the facade retains it. Otherwise, there's the possibility the underlying object's lifecycle is shorter than that of the facade. – marko Aug 06 '12 at 12:36
  • [This](http://stackoverflow.com/q/8706192/500104) should be interesting and/or a dupe. – Xeo Aug 06 '12 at 13:07
  • 4
    If I see this correctly, your only argument against `unique_ptr` is that Google Mock can’t work with them. **This is a bad argument**. A testing framework shouldn’t proscribe fundamental aspects of your design (granted, mocking *always* does this to some extent). – Konrad Rudolph Aug 06 '12 at 13:23

3 Answers3

2

There's nothing wrong with returning a shared_ptr, but I'll try to convince you that this might not be the best option.

By using a smart pointer you gain the advantage of safety, but the users of your API lose the flexibility of using the type of smart pointer that best fits their needs and instead have to always use shared_ptr.

It also depends on how much you value safety over flexibility, but I would personally consider returning a naked pointer and allow the user to use the smart pointer he wants. Of course, if it is necessary that I use shared_ptr for some reason, I will.

Paul Manta
  • 30,618
  • 31
  • 128
  • 208
  • 8
    No naked pointer plx. std::shared_ptr can be constructed from a unique_ptr&&, so the best option IMHO would be to return a unique_ptr. If you want to store the return value in a shared_ptr, then there's no problem, you can still do it. – mfontanini Aug 06 '12 at 12:43
  • @mfontanini Good point, I wasn't aware of that. I am still hesitant, however, since the user could possibly want to use a non-standard smart pointer that cannot be constructed from `unique_ptr&&`. – Paul Manta Aug 06 '12 at 12:48
  • 1
    Non-standard smart pointers that can't be constructed from unique_ptr&& surely can't be constructed from a shared_ptr, so you'd be in the same situation. – mfontanini Aug 06 '12 at 12:50
  • @mfontanini I was saying I would still consider returning a naked pointer unless I was sure I would only need one of the already existing smart pointers. – Paul Manta Aug 06 '12 at 12:52
  • You're right, I sticked to OP's implementation, and forgot that you mentioned naked pointers. – mfontanini Aug 06 '12 at 12:54
  • 5
    You can always `.release()` a `std::unique_ptr`, which makes your argument now completely void; there is *no* reason to *not* return a `std::unique_ptr` for a resource that needs to be managed. – Xeo Aug 06 '12 at 13:02
  • @mfontanini Can I please see an example of constructing a shared_ptr from a unique_ptr? Thanks! – Baz Aug 06 '12 at 13:04
  • `std::shared_ptr a = std::unique_ptr(new int(15));`. That will do it. – mfontanini Aug 06 '12 at 13:05
  • 2
    @Baz: `std::shared_ptr sp(std::move(up));` – Xeo Aug 06 '12 at 13:05
2

The key question is: what are the semantics of the returned pointer?

  • if the returned parent/child/properties object has a lifetime independent of the returning (presumably, in some sense, owning) object, it's reasonable to return shared_ptr: this indicates that the caller and callee have equal rights to decide the object's lifetime

    std::shared_ptr<IChild> child = parent->getFirstChild();
// now I can keep child around ... if parent is destroyed, one
// orphaned subtree is magically kept around. Is this desirable?

  • if the returned object has a lifetime dependent on the callee's own lifetime, then:

    • shared_ptr will wrongly suggest it's meaningful for the caller to extend the returned object's lifetime beyond that of the callee
    • unique_ptr will wrongly suggest transfer of ownership
    • raw pointer doesn't explicitly make any misleading promises, but doesn't give any hint about correct use either

So, if the caller is just getting a working reference to your object's internal state, without either transfer of ownership or extension of object lifetime, it doesn't suggest using any smart pointer.

Consider just returning a reference.

Useless
  • 64,155
  • 6
  • 88
  • 132
  • No, you do not use `weak_ptr` to break cycles. Cycles in managed resource makes no sense. How can you own a pen and the pen own you at the same time? A child should never own its parent. Also see my comment on the question. – Xeo Aug 06 '12 at 13:06
  • so `getFirstChild` and `getParent` both return `shared_ptr`, but neither use it internally? – Useless Aug 06 '12 at 13:07
  • `getFirstChild` shouldn't return an *owning* pointer, it should just return a reference. I don't think it makes sense for the parent to share or even yield its ownership of the child objects. – Xeo Aug 06 '12 at 13:09
  • I agree the ownership semantics are just wrong for `shared_ptr` anyway here - I'll remove the `weak_ptr` note for clarity – Useless Aug 06 '12 at 13:10
  • "if the caller is just getting a working reference to your object's internal state" -- smart pointers aren't for that, btw. They're explicitly for ownership semantics. Other than that, the answer looks good now, +1. – Xeo Aug 06 '12 at 13:12
0

shared_ptr is fine, but it does provide some limitation to the end user, such as C++11 support. A raw pointer, or a trait that allows them to tailor the smart pointer, may provide more flexibility to the end user.

Regardless of the pointer, I suggest being careful with the semantics introduced by the implementation. With the current implementation, with a new wrapped being instantiated for every accessor call, equivalence checks will fail. Consider the following code:

auto child = parent->getFirstChild();
if ( parent == child->getParent() ) // Will be false, as they point to different
                                    // instantiations of Parent.
  ...

if ( child->getParent() == child->getParent() ) // False for the same reason.
  ...

auto sibling = child->getNextChild();
if ( parent == sibling->getParent() ) // Also false for the same reason.
  ...

Also, when using std::shared_ptr, it can be worthwhile to consider using std::make_shared to reduce some of the overhead that occurs with the allocations.

Tanner Sansbury
  • 51,153
  • 9
  • 112
  • 169