-4

I'd like to know which is the best practice writing a class managing some resources, is it better if we explicitly give that class unique ownership or just do it with shared ownership ?

unique ownership

std::unique_ptr<Resource> resource;
Resource* resource_ptr = resource.get();
resourceManager.add(std::move(resource));
resource_ptr->doStuff();

VS

shared ownership

std::shared_ptr<Resource> resource;
resourceManager.add(resource);
resource->doStuff();
underscore_d
  • 6,309
  • 3
  • 38
  • 64
thuny
  • 1
  • I'm not sure what you are trying to achieve here. If you add a resource to `resourceManager` then why do you call `doStuff()` outside of the resource manager? This doesn't make much sense to me. – freakish Jul 03 '17 at 09:54
  • @freakish well the rsrc manager owns the resources but i might wanna update or modify some resource while its owning it – thuny Jul 03 '17 at 09:57
  • 1
    'Which of these things do I need' without context of the situation in which a solution is needed is a completely meaningless question. 'Best practices', by definition, must be applied to something concrete and practical. – underscore_d Jul 03 '17 at 09:58
  • 2
    If one was always better than the other why would the other exist? – Rotem Jul 03 '17 at 09:59
  • It's unclear what you're asking, but whatever it is is almost certainly a duplicate of [C++ shared\_ptr vs. unique\_ptr for resource management](https://stackoverflow.com/questions/23016854/c-shared-ptr-vs-unique-ptr-for-resource-management) – underscore_d Jul 03 '17 at 10:06

2 Answers2

4

Your code samples do not really make much sense. Regardless, I'll answer your more general question...

is it better if we explicitly give that class unique ownership or just do it with shared ownership ?

Firstly, ask yourself: "do I need dynamic memory allocation?". A lot of times you might not need it - prefer values to pointers and try to use the stack.

If you do need dynamic memory allocation, ask yourself "who will own the allocated memory/object?".

  • If you only need a single owner (which is very likely), you should use std::unique_ptr. It is a zero cost abstraction over new/delete. (A different deallocator can be specified.)

  • If you need shared ownership, you should use std::shared_ptr. This is not a zero cost abstraction, as it uses atomic operations and an extra "control block" to keep track of all the owners.

tl;dr: prefer values to dynamic allocation. Prefer unique_ptr to shared_ptr unless you have a good reason to use the latter.

Vittorio Romeo
  • 90,666
  • 33
  • 258
  • 416
  • 1
    Yeah, and if observers need to use things managed by the owner, they can use references, so long as it's guaranteed that they'll never try to do anything with those references after the original object has died (if it ever does). – underscore_d Jul 03 '17 at 10:09
  • Why the downvote? Happy to improve the answer if there's something wrong/unclear. – Vittorio Romeo Jul 03 '17 at 12:01
  • You are not addressing the question, his code sample makes sense (its poor design, but it makes sense, its valid C++ code)... the kind of example this question gives is exactly a situation in which share_ptr should be used, don't go blindly quoting effective C++ – George Jul 03 '17 at 12:06
  • 1
    @George: I honestly don't understand what the OP is trying to do in his/her code samples. How am I not addressing the *"is it better if we explicitly give that class unique ownership or just do it with shared ownership?"* question? And... I never read Effective C++, sorry. All the words in my answers were produced by my brain. – Vittorio Romeo Jul 03 '17 at 12:08
  • No it doesn't his question is clear from the code: I have a pointer to a ressource, I pass this pointer to an object but also manage it from outside the object, what pointer should I use... If you take 1 second to look at ops code you will see that code bit one and code bit 2 accomplish the same thing but code bit 1 casts the uniq_ptr to a raw one thus making it unsafe whilst code bit 2 has nothing particularly wrong about it – George Jul 03 '17 at 12:09
  • I agree, its not a fully fledged piece of working code but from the context OP gives you can sort of piece together a better answer than simply copy-pasting C++ best practice, from your answer it sounds like uniq_ptr is prefered, but out of the two code examples the one with a shared_ptr is the one that doesn't exhibit any flaw here – George Jul 03 '17 at 12:11
  • 1
    @George If that raw pointer is only used in the same scope and is guaranteed not to escape to any point where the `unique_ptr` may have deleted the resource, then I don't really see a problem with the `unique_ptr` option; sometimes complications mean you need to use `.get()` momentarily in order to avoid even more convoluted code. However, also, I think it is valid to question whether the OP really needs to choose between only one of these two options anyway - whether dynamic allocation and pointers are required at all. – underscore_d Jul 03 '17 at 12:11
  • 1
    @George: I don't know what `resourceManager.add(...)` is doing or the lifetime of `resource_ptr`. If the first example was in a self-contained scope then it would be safe. Also, what if his/her current design requires shared ownership, but could be improved to only require unique ownership? You're making too many assumptions from 7 lines of code without any context. – Vittorio Romeo Jul 03 '17 at 12:12
  • @underscore_d If those precautions are taken the unique_ptr should be a shared_ptr and the raw pointer a weak_ptr... using raw pointers is a bad pattern and should be avoided since it doesn't communicate intent (Well, generally speaking, obviously there are loads of situation when you can use them, but this doesn't seem like one of them) – George Jul 03 '17 at 12:13
  • @George: I admit I did copy-paste parts of this answer from one of my previous answers, but it's all my work. I did not "blindly quote" any existing book/article or "copy-pasted" someone else's words without thinking about it. I stand by what I have written in this answer and I think it is valuable information for the OP, regardless of whether he/she needs ownership in his/her design. – Vittorio Romeo Jul 03 '17 at 12:14
  • @Vittorio Romeo Then comment on the fact that design with shared ownership is bad and explain why (ore reference a question/blog which explains why shared owernship is arguably a bad idea in most of the time) – George Jul 03 '17 at 12:14
  • 1
    @George: I already gave a very compelling reason to avoid unnecessary shared ownership in my answer: it's not a zero-cost abstraction. You're paying extra costs for no reason. This is a fact. I also think that it makes code harder to understand as there is no clear ownership hierarchy, but I felt like that reason was too subjective to include in the answer. – Vittorio Romeo Jul 03 '17 at 12:16
  • 1
    @George Yes, that is the more paranoid option. However, if we have full control of the code and are careful not to leave any potential for accidents, then I think `shared_` and `weak_ptr` are unnecessary overheads in such a case when `.get()` suffices for a quick one-off use. Still, people can use whichever option they prefer. – underscore_d Jul 03 '17 at 12:16
  • @ underscore_d That logic will lead to the conclusion that smart pointers aren't needed at all, and they aren't. But in truth most time we don't have full control over the code and we make mistakes and forget what we wrote or we have to edit code mangled by a combination of 5 other people before us... hence why its good to express concetps like resource ownership as best as possible rather than relay on the programmer to "know" he "wasn't supposed to do X or the whole thing would fall appart" – George Jul 03 '17 at 12:18
  • 1
    @George No, my logic won't lead there at all. Types that clearly express ownership semantics should absolutely be used in any code that represents an interface. My point is that using things like `.get()` as an implementation detail in highly localised scopes is not the huge crime you seem to think and doesn't necessarily justify the overhead of how you propose to do it. I guess we'll have to agree to disagree. – underscore_d Jul 03 '17 at 12:22
  • What 'overhead' ? The 'overhead' of an extra atomic int ? – George Jul 03 '17 at 12:27
  • 1
    @George: sigh. It has a signficant overhead - example: http://www.modernescpp.com/index.php/memory-and-performance-overhead-of-smart-pointer - Use Java or any other garbage-collected language if you don't care about wasting performance... – Vittorio Romeo Jul 03 '17 at 12:28
  • 1
    Also, the textual overhead of having to `#include` another header and add a bunch of code that uses it, even if (again: in appropriate, narrow cases) all the extra typing amounts to no semantic difference or benefit. – underscore_d Jul 03 '17 at 12:31
  • @underscore_d share_ptr is in the same header as uniq_ptr.... also "extra typing" is not something I consider an argument. Alas, when it comes to arguing with people who dislike correct usage of smart pointers I know its a battle I can't win, so I agree, lets end it here. – George Jul 03 '17 at 12:35
  • Having to include another big header and type a bunch of templated code is a cost, even if you don't mind it. Also, stop painting us as _"people who dislike correct usage of smart pointers"_, which is simply not true; taking a really snobby attitude to anyone who dares to present an alternative perspective is bad enough, but acting like we don't value smart pointers and go around indiscriminately telling people not to use them is completely disengenuous. – underscore_d Jul 03 '17 at 12:36
-1

In your case shared ownership is what makes sense:

std::unique_ptr<Resource> resource;
Resource* resource_ptr = resource.get();
resourceManager.add(std::move(resource));
resource_ptr->doStuff();

You are passing the pointer on to the manager but than calling an operator on the underlying object outside the resource manager... that is plain bad, for all you know resource_ptr might now be a pointer to garbage since ressourceManager destroyed the unique_ptr you moved into it.

With shared ownership:

std::shared_ptr<Resource> resource;
resourceManager.add(resource);
resource->doStuff();

You are 100% guaranteed that resource is still there when you call "doStuff"... well, unless you cast the shared_ptr to a raw one and delete it in resourceManager, but that would be bad practice.

People will often say uniq_ptr is prefered over share_ptr because using uniq_ptr leads to better design (e.g. not having a RessourceManager class but also managing resources outide of it :P...) however a good rule of thumb to go by is asking yourself:

"Do I need to get a raw pointer from this smart pointer at any point in time ?" If the answer is yes, you are likely using the wrong smart pointer, since casting it to a raw pointer invalidates all safety guards the smart pointer has in place to help you write predictable code.

George
  • 3,521
  • 4
  • 30
  • 75