3

I have some questions related to the use of a shared_ptr pointing to a base class. Their answers influence each other, and for all three I need the same code snippet to set the context in as minimal a way as possible, like this (all the questions relate to Foo and its held_object_):

#include <memory>
#include <utility>

class Bar  // interface for Scene (see linked question)
{
public:
  virtual ~Bar() = 0;
  // more virtual methods to work with Scene
};

Bar::~Bar() = default;

class Baz : public Bar // interface for Foo
{
public:
  virtual ~Baz() = 0;
  // more virtual methods to work with Foo
};

Baz::~Baz() = default;

class ConcreteBaz : public Baz // actual thing I'm acquiring and releasing
{
// overrides and whatnot
};

class Foo
{
public:
  void acquire(const std::shared_ptr<Baz>& object) {};
  void release(/* actually takes a place to release into */) {};

private:
  std::shared_ptr<Baz> held_object_;
};

int main()
{  
  auto foo = std::make_unique<Foo>();
  auto p_foo = foo.get();

  while (/* condition */)
  {
    auto cbaz = std::make_shared<ConcreteBaz>();
    // Scene gets a copy here

    p_foo->acquire(cbaz);
    // do physical things to cbaz
    p_foo->release(/* actually takes a place to release into */);

    // do other physical things, then acquire cbaz again

    p_foo->acquire(cbaz);
    // do physical things to cbaz
    p_foo->release(/* actually takes a place to release into */);
  }
}

As you can see cbaz is a pointer to a polymorphic hierarchy that can work with both Scene and Foo. ConcreteBaz actually represents a physical entity which a Foo can, as stated in the code, take ownership of (it's a shared_ptr because both Scene and Foo own it, as per this. I removed details of Scene here because it's OT).

Questions

1) How do I initialize held_object_? Can I do it with a single allocation?

In reality, foo doesn't own cbaz until it (physically) gets it, therefore the constructor should initialize the pointer to nullptr. Initially I wanted to use make_shared as per this but I read here that it value-initializes, meaning if I were to do

Foo::Foo()
    : held_object_ {std::make_shared<Baz>()} // equivalent to std::make_shared<Baz>(nullptr)
{
}

the compiler would complain with error: invalid new-expression of abstract class type. This would thus become a duplicate of this question but the accepted answer either leaves the shared_ptr uninitialized or creates a further unique_ptr to... basically it's unclear how I would apply it here, and I don't think I can call reset in the constructor (?).

Should I instead be explicit and say

Foo::Foo()
    : held_object_ {std::shared_ptr<Baz> {nullptr}}
{
}

without caring for the double allocation which make_shared avoids? At this point wouldn't it be almost exactly identical to just : held_object_ {} (move ctor vs. default ctor aside)? EDIT: Can I do it with a single allocation as well, like make_shared does?

2) How do I manage held_object_?

Right now to get ownership of cbaz after calling acquire I eventually get into a call for method

void Foo::setHeldObject_(std::shared_ptr<Baz> object)
{
  this->held_object_ = std::move(object);
}

however in release I will have to destroy the owning shared_ptr (to then set it again come next loop) and make the state of my Foo instance coherent with its physical state IRL. This had me thinking of using std::shared_ptr::reset (before even reading the previous related answer) because I would replace the pointee with nullptr if I called setHeldObject() and set cbaz otherwise. However I can't figure the correct syntax to the method's body as:

  • this->held_object_.reset(object); is obviously wrong, as I want to manage the pointee, not the pointer (and doesn't therefore compile);
  • this->held_object_.reset(&object); looks wrong, and yields error: cannot convert ‘std::shared_ptr<Baz>*’ to ‘Baz*’ in initialization
  • this->held_object_.reset(object.get()); also seems wrong because I don't think I'm upping the user_count this way (it does compile though).

EDIT: I believe it should be possible to do it with the same method call, giving or not giving the argument. Would that be possible with reset?

3) There are really only two owners

The main function I wrote here is actually a Process class' run method, but having it defined like this makes it so the use_count bumps to 3 by the time Scene and Foo have their own shared_ptr. From when I make_shared onwards, I'm inside a loop, moreover the logic says there should be only two owners. Is this a use case for weak_ptr where it could make sense to do something like:

int main()
{  
  auto foo = std::make_unique<Foo>();
  auto p_foo = foo.get();

  while (/* condition */)
  {
    auto cbaz = std::make_shared<ConcreteBaz>();
    auto p_cbaz = std::weak_ptr<ConcreteBaz> {cbaz};
    // Scene gets a && via std::move here

    p_foo->acquire(p_cbaz.lock()); // this instantly gets converted to a shared_ptr EDIT: BUT IT DOESN'T INCREASE THE USE COUNT
    // do physical things to cbaz
    p_foo->release(/* actually takes a place to release into */);

    // do other physical things, then acquire cbaz again

    p_foo->acquire(p_cbaz.lock()); // this instantly gets converted to a shared_ptr and would be nullptr had I moved in the first acquire call EDIT: BUT IT DOESN'T INCREASE THE USE COUNT
    // do physical things to cbaz
    p_foo->release(/* actually takes a place to release into */);
  }
}

or is there a better way?

aPonza
  • 492
  • 4
  • 10
  • `held_object_ ` will be initialized properly, don't sweat this, default initialize it. Use std::move, yes. That's the proper way of getting the counter. and `reset(nullptr)` to release it. Third question, that's opinion based and depending on the rest of the design. May not really matter. – Matthieu Brucher Dec 04 '18 at 14:45
  • `held_object_ {std::shared_ptr {nullptr}}` does no allocation at all, and is equivalent to default-initialization. – Quentin Dec 04 '18 at 14:48
  • @Quentin did I misunderstand the purpose of `make_shared` from the linked article? – aPonza Dec 04 '18 at 14:59
  • @LaboratorioCobotica No, you are just overestimating the gains it brings. – Max Langhof Dec 04 '18 at 15:00
  • @LaboratorioCobotica the lesser equivalent of `std::make_shared` would be `std::shared_ptr{new Baz}`, but here you're initializing with `nullptr`. – Quentin Dec 04 '18 at 15:03

2 Answers2

2

How do I initialize held_object_

If you want held_object_ to be nullptr then you don't have to do anything. The compiler generated default constructor of Foo will call held_object_s default constructor leaving it as a null pointer.

How do I manage held_object_

When you need to release the ownership of the pointer you just call reset without any parameters. That effectively does shared_ptr().swap(*this); which leaves you with a null pointer.

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • I really don't have an answer for 3. I'm not sure why you need `use_count` to only be 2. It's okay if the creator of the pointer shares the ownership until it returns and that local copy is destroyed. – NathanOliver Dec 04 '18 at 14:52
  • Because semantically there should only be only two owners, `Foo` and `Scene`. Besides, I read everywhere that "`shared_ptr` is incremented atomically and that's expensive", and since I'm creating said pointer inside a loop I want to do it the right amount of times. – aPonza Dec 04 '18 at 14:55
  • @LaboratorioCobotica If you really only want 2 owners max then one of the objects you give the pointer to you should use `std::move(cbaz)` so it move it from the calling function into the owning object. Then your left with a null pointer and `use_count` will only be 2. – NathanOliver Dec 04 '18 at 14:59
  • I can't do that: were I to `move` into `scene` I would lose a way to give ownership to `foo`, and conversely I actually have two calls to `acquire` within the loop (same physical object, I have to do stuff to it in a place and than other things in another), so I need the logic to be there, that's why I was thinking of `weak_ptr` – aPonza Dec 04 '18 at 15:21
  • @LaboratorioCobotica The weak pointer gains you nothing. Either copy into `scene` and move into `foo` or have a shared pointer in `run`. If you could call acquire multiple times then you probably need the latter. A single extra atomic increment/decrement isn't going to do much. – NathanOliver Dec 04 '18 at 15:24
  • I added the looping logic which was previously missing, this should make it more clear – aPonza Dec 04 '18 at 15:36
  • 1
    @LaboratorioCobotica With the way you loop looks and what you want to do I can't see how you could have less than 3 owners. `run`, scene` and `foo` are all using the pointer at the same time. I guess you could give `scene` the pointer, get a reference back to it from `scene`, copy that into `foo` so now `scene` and `foo` own the pointer and `run` has a reference to it. It'd be silly to do but it gets the use count down. I wouldn't really worry about it though. More than likely that extra atomic operation is going to get lost in the noise of the loop. – NathanOliver Dec 04 '18 at 15:51
  • I just saw weak_ptr doesn't work like I thought in my example (it doesn't increment the use_count, mainly), so I'll follow your advice and worry about it in an optimization phase, even though the design isn't quite perfect. I'll just edit the example to make it compile in case someone wants to try it. – aPonza Dec 05 '18 at 08:24
0

1) How do I initialize held_object_?

You can just do this:

Foo::Foo()
    : held_object_ {nullptr}
{
}

But that's of course equivalent to : held_object_ {}, albeit slightly more explicit in your intent. Another alternative would be to have

class Foo
{
  // ...
private:
  std::shared_ptr<Baz> held_object_ = nullptr;
};

2) How do I manage held_object_?

Your first version with the std::move is just fine. Do note that you only partake in ownership this way, you don't get exclusive ownership (which is why I would not name the method acquire).

3) There are really only two owners

I don't think you can get a real answer to this one, as it's both dependent on your exact situation and (even given that) somewhat opinion-based. Also, don't sweat the use count unless you're actually in it for performance. Writing different code because you are worried about some minute performance detail (cost of reference count in shared_ptr) is premature optimization unless you have very good reasons to believe (or proof) that it will be a problem. Do you do this in an inner loop? If not, a spurious refcount up/down won't be felt.

Max Langhof
  • 23,383
  • 5
  • 39
  • 72
  • Why even declaring it? Just remove the clutter. – Matthieu Brucher Dec 04 '18 at 14:53
  • @MatthieuBrucher Because e.g. Lint will yell at you for not initializing your members. By explicitly initializing like shown, there is no doubt that what happens is intended. Of course, you could just default-initialize with `= nullptr` at the declaration instead. – Max Langhof Dec 04 '18 at 14:54
  • Lint may tell you this, and clang-tidy says exactly the opposite. I'm for readability, not clutter. – Matthieu Brucher Dec 04 '18 at 14:55
  • @MatthieuBrucher That was copied from the OP. Re Lint: That's your opinion, which is fine but not absolute. – Max Langhof Dec 04 '18 at 14:55
  • I am doing it in a loop, actually, as per `The part with acquire and release is actually in a loop` somewhere up top. Should have made it more evident – aPonza Dec 04 '18 at 15:02