0

Below is the simplified version of a class that I am trying to design

class Class {
 public:
     Class(std::unique_ptr<int>&& ptr): ptr_(std::move(ptr)) {}
 private:
     // works only if I change to std::shared_ptr or remove const qualifier
     const std::unique_ptr<int> ptr_;
};

int main() {
  std::unique_ptr<int> ptr = std::make_unique<int>(1);
  Class c(std::move(ptr)); // Compiles
  // I need to construct a new instance d, but c is not needed anymore,
  // anything c owns can be nullified.
  Class d(std::move(c)); // Does not compile
  return 0;
}

I can not construct an instance d from the instance c due to the following error message:

Copy constructor of 'Class' is implicitly deleted because field 'ptr_' has a deleted copy constructor

However, if I change the ptr_ member to be of type const std::shared_ptr<int>, then everything works.

Is there a way to have a single constructor for the Class, which would support:

  • Constructing the class instance directly providing the arguments it needs
  • Constructing the class instance from another class instance, possibly destroying another instance in the process (i.e. c(std::move(d));
  • Allowing the class to have a member of type const unique_ptr

?

EDIT: constructing an instance d using std::move(c) also doesn't work:

class.cc:17:23: error: use of deleted function ‘Class::Class(Class&&)’ 17 | Class d(std::move(c)); | ^ class.cc:5:7: note: ‘Class::Class(Class&&)’ is implicitly deleted because the default definition would be ill-formed:

So far two solutions work for me, neither are perfect from readability point of view:

  1. Remove const qualifier:

    std::unique_ptr<int> ptr_;

    The problem is that this reads as: "ptr_ member might be modified when calling public methods"

  2. Change to shared pointer:

    const std::shared_ptr<int> ptr_;

    The problem is that this reads as: "we have multiple pointers pointing to the same data"

Any way to improve the design which wouldn't have the problems outlined in (1) and (2)?

mercury0114
  • 1,341
  • 2
  • 15
  • 29
  • 2
    `Is there a way to have a single constructor for the Class, which would support:` No. – tkausl Jan 15 '23 at 13:08
  • 1
    Depends on your use case. Does every class instance need ownership of that pointer? – πάντα ῥεῖ Jan 15 '23 at 13:08
  • @πάνταῥεῖ I need to construct the class only once, and the pointer will be owned only by a single class. – mercury0114 Jan 15 '23 at 13:09
  • wait, do you mean "instance" when you say "class", @mercury0114? – Marcus Müller Jan 15 '23 at 13:10
  • @mercury0114 After copying, is `d.ptr_` supposed to point to the same `int` object as `c.ptr_`, to a different one (but with the same value?), or should it be a null pointer? – user17732522 Jan 15 '23 at 13:11
  • @mercury0114 then a copy constructor doesn't make any sense at all. – πάντα ῥεῖ Jan 15 '23 at 13:12
  • @user17732522, at the end d.ptr_ should point to one object, all pointers before can be nullified. – mercury0114 Jan 15 '23 at 13:13
  • 1
    @mercury0114 A copy constructor shouldn't modify the source object, so it can't nullify `c.ptr_`. That's what a move constructor is for (and it already does exactly that). – user17732522 Jan 15 '23 at 13:14
  • @user17732522 Suppose I constructed an instance `Class c`. Now I want to write `Class d(c)`. After instance `d` is constructed, I don't need `c` anymore, anything c owns can be nullified. Is there a way to have any constructor which would allow me to write `Class d(c)`? – mercury0114 Jan 15 '23 at 13:15
  • 2
    @mercury0114 The canonical way of telling the intent that you don't need `c` any more afterwards is to pass `std::move(c)` instead of `c`, which will cause the move constructor to be used, which is already defined implicitly: `Class d(std::move(c));`. – user17732522 Jan 15 '23 at 13:17
  • @mercury0114 If you make the syntax `Class d(c)` work somehow (which in theory is possible), then this breaks all assumptions that libraries and developers make on how copy constructors behave. Using a class with such a copy constructor will break e.g. standard library algorithms, make it impossible to use the type in standard library containers and other classes, etc. – user17732522 Jan 15 '23 at 13:18
  • 4
    *Can you design a copy constructor when the class has a member of type const std::unique_ptr?* **Yes**, you can do that. *Is there a way to have a single constructor for the Class...?* **No**, you'll have to write the copy constructor yourself, and consider writing a move constructor, and probably will want/need a copy assignment and move assignment. – Eljay Jan 15 '23 at 13:29
  • If you need to copy a unique_ptr, then perhaps what you need is not a unique_ptr, but either a different kind of smart pointer or no pointer at all (`std::optional` or just directly ``). – n. m. could be an AI Jan 15 '23 at 13:42
  • A unique pointer models something called ownership, it models who should cleanup the memory the unique_ptr points to. If unique pointer was copyable that would mean an object could be deleted twice and that will not do. So that's why unique_ptr is not copyable. You can't have the cake and eat it too. So you can give a class with a unique_ptr a move constructor but not a copy constructor. If you want shared ownership then there is std::shared_ptr/std::make_shared but that should be used with care too (you can build cyclic lifetme dependencies without proper design) – Pepijn Kramer Jan 15 '23 at 13:55
  • 1
    Can you explain how you expect such a copy constructor to work, in plain English. Keep in mind that there can only be one `unique_ptr` that's referencing an object. If you can explain how you expect a copy constructor to work, in plain words, then you should be able to implement a copy constructor for an object with a `unique_ptr`, accordingly. – Sam Varshavchik Jan 15 '23 at 13:57
  • Not sure how copy/move assignment would work when you have a `const` member unless you `const_cast` away the `const`. In general I find `const` members to be a code smell. – WBuck Jan 15 '23 at 13:59
  • You've already encapsulated the member as a `private` member variable. The use of `const` here is senseless. If you want to ensure the classes internal state is not modified in its member functions declare said member functions as `const`. – WBuck Jan 15 '23 at 14:03
  • 2
    "_constructing an instance d using std::move(c) also doesn't work:_": Yes, sorry about that. I didn't notice you made the pointer `const` as well. Making the pointer `const` means that you intent it to be impossible for it to relinquish ownership. If you want to be able to transfer ownership, then it must not be `const`. – user17732522 Jan 15 '23 at 14:03
  • @WBuck `In general I find const members to be a code smell`, lol, const members are great, if you can make something const, you should. That protects engineers from accidentally modifying the members when they shouldn't. – mercury0114 Jan 15 '23 at 16:20
  • `declare said member functions` - class might have other members that are not const, and the methods might legitimately modify those other members, so the methods can't be const. – mercury0114 Jan 15 '23 at 16:24
  • 3
    @mercury0114: "*const members are great, if you can make something const, you should*" And making members const has consequences. The object cannot be assigned to at all. This makes interacting with such types way harder than it needs to be. – Nicol Bolas Jan 15 '23 at 16:35
  • 1
    @mercury0114: "*The problem is that this reads as: "ptr_ member might be modified when calling public methods"*" And that's true. Moving the object must cause the original to be modified. Move constructors are public functions; therefore some public APIs modify the object. "*The problem is that this reads as: "we have multiple pointers pointing to the same data"*" That's also true: if you copy the pointer, you have two objects pointing to the same data. Your problem is ultimately that you don't want to accept the consequences of the decisions you're making in your code. – Nicol Bolas Jan 15 '23 at 16:37
  • @mercury0114: Your overall problem is that you don't want to engage with this simple question: what do you want it to *mean* when you copy one of these objects? Do you want the two objects to point to the same object or to different ones? What do you want it to mean when you move the object? Do you want the old object to be transferred over (thus modifying the old object's value)? – Nicol Bolas Jan 15 '23 at 16:40

1 Answers1

4
  • Constructing the class instance from another class instance, possibly destroying another instance in the process (i.e. c(std::move(d));
  • Allowing the class to have a member of type const unique_ptr

This intersection of desires is inherently contradictory.

Moving is a modifying operation. If the object is const, you cannot move from it.

The entire purpose of unique_ptr is that there is one instance in the program which uniquely owns (and will destroy) the object it points to. Therefore, it cannot be copied, because a copy is a non-modifying operation. If it could be copied, then two objects would try to own the same object.

You cannot copy from a unique_ptr. You cannot move from a const object of any kind. Therefore, if your object has a const unique_ptr, it cannot get a pointer from some other instance of that object.

By declaring a member to be a const unique_ptr<T>, you are also declaring that the object can be neither copied nor moved. Those are the consequences of your code choices.

So you're going to have to decide what is more important: the member being const or the ability to copy/move from this object.

Broadly speaking, const members create more problems than they usually solve. If the member is private, then that is usually good enough protection; users with direct access to the class should be able to know what each such function should and should not modify. And if they make a mistake, the code in which that mistake can appear is pretty limited.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • `Broadly speaking, const members create more problems than they usually solve` - I don't agree with this claim, but the rest of the accepted answer makes sense, thanks! – mercury0114 Jan 15 '23 at 17:06