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?

Improve this question
4
  • By the way, what exactly is OtherType? Is it a base class with virtual member functions? Do you need subtype polymorphism? Commented Sep 23, 2010 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? Commented Sep 23, 2010 at 11:38
  • Interfaces are a prime example for unique_ptr usage. No further questions. Commented Sep 23, 2010 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. Commented Sep 23, 2010 at 11:54

4 Answers 4

Reset to default
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;
}
Improve this answer
Sign up to request clarification or add additional context in comments.

25 Comments

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: 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.
@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.
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.
@roysc In line 23, auto r = ... makes a copy, even if ... is returned by reference. Try auto& r instead.
|
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>();
}
Improve this answer

6 Comments

I was following advice from another answer from here stackoverflow.com/questions/3759119/… where AshleysBrain suggested using unique_ptr to store in a collection
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.
@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.
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.
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.
|
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.

Improve this answer

1 Comment

YEah my mistake, but you get the jist I hope
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).

Improve this answer

1 Comment

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.