4

If my class SomeType has a method that returns a element from the map (using the key) say

std::unique_ptr<OtherType> get_othertype(std::string name)
{
   return otMap.find(name);
}

that would enure the caller would recieve a pointer to the one in the map rather than a copy? Is it ok to do this, or would it try and call the copy constructor (and fail as it has been removed) because it is being returned?

Assuming I must use unique_ptr as my map items.

UPDATE::

After trying to implement the code, it seems that unique_ptr and std:map/:pair dont work together in gcc 4.4.4, pair just didnt like unique_ptr as a type parameter. (see Can't create map of MoveConstructibles).

I changed the ptr to std::shared_ptr and it all worked.

I suppose I can use the same code with the shared pointer?

Community
  • 1
  • 1
Mark
  • 1,538
  • 5
  • 24
  • 31
  • By the way, what exactly is `OtherType`? Is it a base class with virtual member functions? Do you need subtype polymorphism? – fredoverflow Sep 23 '10 at 11:33
  • It will be a base class with pure virtual method like an interface. I would just need to call one of the pure virtual methods no need for casting down. Does this change things? – Mark Sep 23 '10 at 11:38
  • Interfaces are a prime example for `unique_ptr` usage. No further questions. – fredoverflow Sep 23 '10 at 11:46
  • Thanks Fred, its better to spend time understanding and using the best tools for the job, that blindly building on a house of sand based on invalid assumtions. – Mark Sep 23 '10 at 11:54

4 Answers4

14

The model of unique_ptr is transfer of ownership. If you return a unique_ptr to an object from a function, then no other unique_ptr in the system can possibly refer to the same object.

Is that what you want? I highly doubt it. Of course, you could simply return a raw pointer:

OtherType* get_othertype(const std::string& name)
{
    return otMap.find(name)->second.get();
}

Thus, the client has access to the object, but the map still owns it.

The above solution is rather brittle in case there is no entry found under the name. A better solution would be to either throw an exception or return a null pointer in that case:

#include <stdexcept>

OtherType* get_othertype(const std::string& name)
{
    auto it = otMap.find(name);
    if (it == otMap.end()) throw std::invalid_argument("entry not found");
    return it->second.get();
}

OtherType* get_othertype(const std::string& name)
{
    auto it = otMap.find(name);
    return (it == otMap.end()) ? 0 : it->second.get();
}

And just for completeness, here is Anthony's suggestion of returning a reference:

OtherType& get_othertype(const std::string& name)
{
    auto it = otMap.find(name);
    if (it == otMap.end()) throw std::invalid_argument("entry not found");
    return *(it->second);
}

And here is how you return a reference to the unique_ptr inside the map, but let's make that a reference to const, so the client does not accidentally modify the original:

unique_ptr<OtherType> const& get_othertype(const std::string& name)
{
    auto it = otMap.find(name);
    if (it == otMap.end()) throw std::invalid_argument("entry not found");
    return it->second;
}
fredoverflow
  • 256,549
  • 94
  • 388
  • 662
  • 1
    So what is the standard way to do this? I would want the callee to have a pointer to the objet but the map still be the owner. However I wouldnt want to use a raw as the callee might not dispose of it? What would happen then if I delete it fromt he map, his raw would be pointing to nothing? – Mark Sep 23 '10 at 11:09
  • 1
    @Mark: Is the object guaranteed to be in the map? If so, then you can return a reference instead of a pointer and that way the semantics of the call are more explicit: I will give you access but not ownership. – David Rodríguez - dribeas Sep 23 '10 at 11:12
  • The way the system is designed if the object wasnt there it would be an exception. The reference idea sounds like the best option. – Mark Sep 23 '10 at 11:15
  • @Mark: The callee is not supposed to dispose of it. In fact, that would be a very bad idea, because the `unique_ptr` is responsible for disposal of the object! If the client holds on to the raw pointer after the `unique_ptr` has been destroyed and then dereferences the raw pointer, you get undefined behavior. Is the client supposed to store the pointer he gets from `get_othertype` somewhere? Do you want unique ownership or shared ownership? References don't solve this problem at all, the client can hold on the the referred object just as well if he wants to. *Does he want to?* is the question. – fredoverflow Sep 23 '10 at 11:15
  • @FredOverflow: The caller (I had said callee before sorry) would just be manipulating the object not taking ownership, there is only one owner and that is the map. I am just thinking why the caller wouldnt use a smart pointer, why should he use raw pointers? – Mark Sep 23 '10 at 11:17
  • @Mark: So the use case is `foo.get_othertype("bar")->do_stuff_now()` and not `variable_for_later_use = foo.get_othertype("bar")`? Then the raw pointer example code I posted in my answer is exactly what you want. Why would you use a smart pointer here? What benefits would it give you? What kind of smart pointer would you suggest? I can't think of a scenario that works without changing the underlying map structure from `unique_ptr` to `shared_ptr`, and I see little value in doing that. – fredoverflow Sep 23 '10 at 11:20
  • @Fred: Well I might want to hold it as a local pointer for the scope of the caller function (but not more that that), rather then accessing it (If I have to make a few manipulations) – Mark Sep 23 '10 at 11:23
  • @Mark: Storing a local copy is not problematic at all. The only thing you should not do is store the pointer in a global variable or somewhere else where it is accessible long after the `unique_ptr` has vanished. – fredoverflow Sep 23 '10 at 11:25
  • I am fairly new to C++, just as a cavaet. I see now that the raw pointer returned would be stored in the stack frame of the caller and would pop at the end. I think I have got it now? So raw pointer returned is the best option. – Mark Sep 23 '10 at 11:26
  • 1
    @Mark: Yes, and "popping" a local raw pointer at the end of a function has no effect whatsoever. Just for clarity, if a local `unique_ptr` is "popped", the object it refers to is destroyed immediately. And if a `shared_ptr` is "popped", the associated reference count is decremented by one, and if it has reached zero, the object is destroyed. And just to reassure you, given the use case that you described, returning a raw pointer seems to be the best option to me. – fredoverflow Sep 23 '10 at 11:30
  • @Fred : Yes I agree, I will be using your solution exactly, thanks for your explanation it has helped me understand how to use unique_ptr properly. – Mark Sep 23 '10 at 11:36
  • @Mark: Good. Now that we know what you need, let's make the solution safer. See my update. – fredoverflow Sep 23 '10 at 11:44
  • 4
    I would suggest that you return a **reference** rather than a raw pointer. It makes it clear that the map owns the object. Of course, in this case you'd better throw if the object isn't there. – Anthony Williams Sep 23 '10 at 12:15
  • @Anthony: I added example code that returns a reference. Feel free to edit according to your taste. – fredoverflow Sep 23 '10 at 12:29
  • SO using a reference is preferred? Is that because its somewhat safer (ie you cant change it) and you cant assing null so we must throw and excpetion? – Mark Sep 23 '10 at 13:15
  • @Mark: Preference is a very subjective matter :-) Anthony prefers the reference solution, I prefer the pointer solution. I suggest you read a good book on C++, learn about the differences between pointers and references, and then decide for yourself. By the way, you could also return a reference to the unique pointer in the map, let me just update my answer... – fredoverflow Sep 23 '10 at 16:27
  • @Mark: Using a reference is preferred because it has [tighter post-conditions](http://stackoverflow.com/questions/1744070/why-should-exceptions-be-used-conservatively/1744176#1744176): the function always returns a reference to a valid object in the map (or it raises an exception). It's simpler to use the object (slightly, no dereference) and easier to understand the ownership at a glance. (cont'd) –  Sep 23 '10 at 16:29
  • If you return a pointer, code that doesn't check for null pointers would ["look wrong"](http://www.joelonsoftware.com/articles/Wrong.html)—we eliminate that entire situation, replacing it with a model that *tells* you when something is wrong rather than silently doing the absolute wrong thing, which is continue with the null pointer as if it wasn't null. –  Sep 23 '10 at 16:32
  • @Roger: And how can you be certain that there always is a valid object? Even if the name exists as a key inside the map, it could be mapped to an unbound `unique_ptr`. In that case, `*(it->second)` leads straight into undefined behavior land. Starting with a (smart) pointer and then ending up with a reference somehow feels wrong to me, but I guess this is an entirely subjective matter. – fredoverflow Sep 23 '10 at 16:34
  • @Fred: If the map stores unique_ptrs, then the *unique\_ptr* is the valid object in the map. However, if you want to additionally check the pointed-to object, that's certainly an option and would be additional semantics. Also note that some smart managers can't store nulls, such as Boost's [pointer containers](http://www.boost.org/doc/libs/1_44_0/libs/ptr_container/doc/tutorial.html#null-values), so it depends what type of smart pointer you have. –  Sep 23 '10 at 16:49
  • Thanks guys, I have 3 options now , raw, ref, or ptr to unique_ptr. As you saw its a preference thing as long as the post conditions are checked for and the caller gets a valid mechanism to access the underlying object. – Mark Sep 23 '10 at 18:43
  • @Fred: Correct me if I'm wrong, but it seems that attempting to actually use the last version (for anything other than an rvalue) will fail, because it requires the use of `unique_ptr(const unique_ptr&)`, which is a deleted function. – scry Sep 06 '13 at 01:22
  • @roysc The `unique_ptr` is neither copied nor moved out of the function. Note the `const&` in the return type! The original is simply passed back by const reference. – fredoverflow Sep 06 '13 at 09:42
  • @FredOverflow Though that makes sense to me, but this [code](http://ideone.com/Np0WTq) fails to compile. Apparently it does use the copy constructor, but why? – scry Sep 06 '13 at 16:26
  • 1
    @roysc In line 23, `auto r = ...` makes a copy, even if `...` is returned by reference. Try `auto& r` instead. – fredoverflow Sep 07 '13 at 11:21
2

What is the type of otMap?

If otMap.find(name) returns a std::unique_ptr<OtherType> as an rvalue then this will work fine. However, ownership of the pointed-to value has now been transferred to the returned pointer, so the value will no longer be in the map. This would imply you were using a custom map type rather than std::map<>.

If you want to be able to have the value in the map and return a pointer to it, then you need to use std::shared_ptr<OtherType> both as the map value type and the return type of get_othertype().

std::map<std::string,std::shared_ptr<OtherType>> otMap;
std::shared_ptr<OtherType> get_othertype(std::string name)
{
    auto found=otMap.find(name);
    if(found!=otMap.end())
        return found->second;
    return std::shared_ptr<OtherType>();
}
Anthony Williams
  • 66,628
  • 14
  • 133
  • 155
  • I was following advice from another answer from here http://stackoverflow.com/questions/3759119/creating-an-object-on-the-stack-then-passing-by-reference-to-another-method-in-c where AshleysBrain suggested using unique_ptr to store in a collection – Mark Sep 23 '10 at 11:03
  • The overhead of `shared_ptr` over `unique_ptr` is small (a reference count). Using `unique_ptr` in this case requires careful thought, and I would strongly recommend against it unless it really does exactly what you want. – Anthony Williams Sep 23 '10 at 11:17
  • 1
    @Ant: I disagree. `unique_ptr` clearly expresses the programmer's intent of the map *owning* the objects, and it makes the code potentially faster. A correct implementation of `shared_ptr` has to take thread-safety into account to get the reference counting right, which makes it potentially a lot slower than `unique_ptr`. – fredoverflow Sep 23 '10 at 11:24
  • 1
    Sure. Putting `unique_ptr` in the map says that the map owns the object. However, it leaves you with the problem of how to access it. If you access it directly e.g. with `otMap[name]->something` then that's OK, but returning a raw pointer is less desirable. The ref count overhead of `shared_ptr` is small unless the object is actually shared between threads. Small enough that I wouldn't worry unless profiling told me otherwise, anyway. – Anthony Williams Sep 23 '10 at 12:13
  • @Anthony: The client could still `get()` a raw pointer from the `shared_ptr`. C++ requires responsible usage, there is no 100% safety. – fredoverflow Sep 23 '10 at 12:17
  • 1
    If they do that that's their problem. If you give them a raw pointer, that's your problem. I don't like APIs that give me raw pointers, as it's not clear who owns it, or what the lifetime is. – Anthony Williams Sep 23 '10 at 12:22
0

otMap.find will return an rvalue and thus this rvalue will be moved, if not RVO'd. But, of course, now your map doesn't have that particular object in. Also, last time I checked, find returns an iterator, not the value type.

Puppy
  • 144,682
  • 38
  • 256
  • 465
0

Would you consider changing the map to hold shared_ptrs instead of unique_ptrs? This will make returning a value much safer. The whole point of unique_ptr is that it is unique (i.e. not shared).

Motti
  • 110,860
  • 49
  • 189
  • 262
  • 2
    The point is unique *ownership*. That doesn't exclude the possibility of other raw pointers pointing to the same object. In that sense it's not much different compared to a container that returns an iterator. – sellibitze Sep 24 '10 at 07:14