2

I have a very specific need to access functionality specific to a derived class of which I get an instance in a unique_ptr at construction of the containing class. This containing class must then move the unique_ptr upcast to its base class to the containing class' base class constructor, where it is finally moved for ownership in that base containing class. Some code should help:

class MemberBase {};
class MemberDerived : public MemberBase { /*some public stuff not in MemberBase*/ };

class MainBase {
  std::unique_ptr<MemberBase> member_;

 public:
  MainBase(std::unique_ptr<MemberBase> member) : member_(std::move(member)) {}
};

class MainDerived : public MainBase {
  MemberDerived* member_derived_;
  // This class proceeds to use MemberDerived-only functions

 public:
  // How to write the initialization list below?
  MainDerived(std::unique_ptr<MemberDerived> member)
    : MainBase(std::move(member)), member_derived_(member.get() /*nullptr!!*/) {}
};

Ignoring the question of overall class design (I'm aware that the coupling to MemberDerived-specific functions in MainDerived is not ideal; this is inherited code that cannot be altered without major refactor), how can I snag the raw pointer before forwarding the unique_ptr to MainBase?

Find below some ideas I have thought of and why I do not think they are great.

  1. Downcast protected accessor:
// Add this method to the protected section of MainBase:
MemberBase* MainBase::get_member() { return member_.get(); }

// Then downcast in MainDerived's c'tor
MainDerived::MainDerived(std::unique_ptr<MemberDerived> member)
  : MainBase(std::move(member)), member_derived_(dynamic_cast<MemberDerived*>(get_member())) {}

This should work but uses dynamic_cast (major downside in and of itself) and of course if someone changes the type passed to the c'tor to something not derived from MemberDerived it will break with no help from the compiler.

  1. Pass in the pointer twice:
// member and member_derived must point to the same object!
MainDerived::MainDerived(std::unique_ptr<MemberDerived> member, MemberDerived* member_derived)
  : MainBase(std::move(member)), member_derived_(member_derived) {}

Besides making for a pretty ugly c'tor signature it would be pretty easy for the user to pass a different pointer for both arguments or do the move before calling get. Furthermore the user is now forced to create a local variable to pass it into both places. Did I mention it's ugly?

  1. Flip initialization order with helper function trickery:
template <typename T>
std::unique_ptr<T> ExtractPointer(std::unique_ptr<T> p, T** target) {
  *target = p.get();
  return std::move(p);
}

MainDerived::MainDerived(std::unique_ptr<MemberDerived> member)
  : MainBase(ExtractPointer(std::move(member), &member_derived_)) {}

Now I was actually a bit surprised that this did not produce any warnings/errors (gcc 5.4.0 with -Wall). Part of me likes this as it seems safe in the sense that it is hard to break but the circuitous initialization of member_derived_ gives me minor chills.

walnut
  • 21,629
  • 4
  • 23
  • 59
Luis
  • 1,210
  • 2
  • 11
  • 24
  • Related to [move-semantics-and-function-order-evaluation](https://stackoverflow.com/questions/24814696/move-semantics-and-function-order-evaluation) – Jarod42 Dec 11 '19 at 10:05

2 Answers2

4

Your second solution is halfway there. Note that the constructor does not need to be public. You can make the 2-argument one private and write a 1-argument public constructor that delegates to it.

class MainDerived : public MainBase {
  MemberDerived* member_derived_;

  MainDerived(std::unique_ptr<MemberDerived> &member, MemberDerived *member_derived)
    : MainBase(std::move(member)), member_derived_(member_derived) { }

 public:
  MainDerived(std::unique_ptr<MemberDerived> member)
    : MainDerived(member, member.get()) { }
};

member must be taken by reference in the private constructor, as the ordering of the evaluations of the arguments of a function call is unspecified. Thus, you can't safely move member from the public constructor into an argument of the private one, as then the ordering of the move and the get() would be unspecified, but we need get() to happen before the move. The easiest fix is for the private constructor to take the member by reference, which does not mutate it. (You can also use {} instead of () to enforce the order of evaluation, but making your code depend on such a delicate construct is not a good idea.)

HTNW
  • 27,182
  • 1
  • 32
  • 60
  • 1
    You don't actually have to release; the delegated constructor can just have 2 arguments. – o11c Dec 11 '19 at 02:44
  • 1
    @o11c No, wait, I don't think that works. In order to have two arguments, you need to move the `unique_ptr` into one of them. But the order of evaluation of function arguments is unspecified, so I don't think there's a guarantee that the call to extract the raw pointer will see the original `unique_ptr` and not the empty one. There's another reason you may want to use 2 arguments, though... – HTNW Dec 11 '19 at 02:52
  • 2
    @HTNW I think left-to-right sequencing is guaranteed if you use list-initialization, i.e. `MainDerived{..., ...}` instead of `MainDerived(..., ...)`. Whether you want to make code-correctness depend on that detail is another question. – walnut Dec 11 '19 at 02:57
1

Since you know that the pointer is to a MemberDerived, you don't need dynamic_cast. A static_cast will do.

If you are worried about making a mistake in the types when something is changed, then just refer to the element type:

MainDerived(std::unique_ptr<MemberDerived> member)
  : MainBase(std::move(member))
  , member_derived_(static_cast<decltype(member)::element_type*>(member_.get())) 
{}

As you mentioned this requires member_ to be protected or a protected getter to exist.

As far as I can tell there is no case where this can lead to undefined behavior, except if someone passes a std::unique_ptr that doesn't actually point to an object of type identical to or derived from it's element_type, in which case use of this std::unique_ptr would itself be unsafe, because it would cause undefined behavior if the instance was destroyed without prior move.


As for your 3. suggested method:

I think this is technically allowed in C++17, because member_derived_ has vacuous initialization, so its lifetime begins when its storage is allocated, not when its initialization finishes.

But the current C++20 draft removes this exception, so that member_derived_'s lifetime will start only after its vacuous initialization, making this use undefined behavior.

walnut
  • 21,629
  • 4
  • 23
  • 59