20

Let's say I have a library which has a Document class. An instance of Document can own several instances of Field. Field has multiple subclasses (for example IntegerField and StringField), and even the API user can subclass it and supply subclass instances to Document (let's say the user is allowed to develop a custom type of data to store in a field).

I'd like to expose the Field instances owned by Document through an API in such a way that users can interact with them, but without transferring ownership.

What is the right way to do this?

I thought about:

  • Exposing a const std::unique_ptr<Field>& reference - this feels quite ugly
  • Exposing a plain Field* pointer - this doesn't feel right because the user might be unsure whether he should delete the instance or not
  • Using std::shared_ptr instead - this feels bad because the ownership is not really shared

For example,

class Document {
private:
    std::map<std::string, std::unique_ptr<Field> > fields;
    // ...
public:
    // ...

    // How is this done properly?
    const std::unique_ptr<Field> &field(const std::string &name) {
        return fields[name];
    }
}

I'm looking forward to your suggestions.
(I also welcome advice about alternative approaches like what @Fulvio suggested.)

Venemo
  • 18,515
  • 13
  • 84
  • 125
  • Why not simply `Bar&` and throw on missing name ? – galop1n Apr 16 '14 at 14:25
  • You forgot an alternative: A `getBar` that gives ownership to the caller, and a `releaseBar` that gives ownership back to `Foo`. It requires that the user of your `Foo` class always releases its ownership when done. – Some programmer dude Apr 16 '14 at 14:27
  • 1
    @Joacim - I didn't forget, but I said in my question that I want to do this without transferring ownership. :) – Venemo Apr 16 '14 at 15:01

7 Answers7

13

I'd return Bar& (perhaps with a const).

The user will need to understand that the reference will be invalidated if the element is removed from the map - but this will be the case whatever you do, due to the single-ownership model.

Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • @MadScienceDreams: `weak_ptr` doesn't give the single-ownership model the OP wants: anyone can use the weak pointer to share ownership. It also adds some overhead that will only be necessary if the user does something weird with the returned reference/pointer. But you're right that it might be a suitable choice in similar situations. – Mike Seymour Apr 16 '14 at 15:05
  • Sorry, for some reason I though that you could make non-onwnership pointers to unique_ptrs, my bad :-P – IdeaHat Apr 16 '14 at 15:07
  • 2
    (From my old comment) This has the benefit of also exposing a C++98 compatible interface, which (if you use the impl idiom) allows you to interface with non-C++11 code. – IdeaHat Apr 16 '14 at 15:09
6

As others have answered from a technical standpoint, I'd like to point you at a different approach and revise your design. The idea is to try to respect the Law of Demeter and don't grant access to object's sub-components. It's a bit harder to do, and without a specific example I can't provide many details but try to imagine a class Book formed of Pages. If I want to print one, two or more pages of the book, with your current design I can do:

auto range = ...;
for( auto p : book.pages(range) )
  {
  p->print();
  }

while abiding by Demeter's you'll have

auto range = ...;
book.print( /* possibly a range here */ );

this is slanted towards better encapsulation as you don't rely upon internal details of the book class and if its internal structure changes, you don't need to do anything to your client code.

  • This is a good idea, but (considering your example) what if the `Page` class could have multiple descendants that can also be supplied by the user? In that case, not all the possible methods are known at design time and thus it might be feasible to expose the objects thsmselves. – Venemo Apr 16 '14 at 18:52
  • Design is always highly contextual so every choice you make may or may not satisfy your requirements. My intention was to point out a design alternative, but as I said, without knowing your requirements/constraints/context, this was meant to be a suggestion to look somewhere else for a **possible** alternative solution. Anyway, if you want to use methods only available in sub-classes you are tiding your client code to specific types bypassing polymorphism. Again, it's hard to reason in terms of Foo and Bar, sorry :) – Fulvio Esposito Apr 17 '14 at 11:57
  • I appreciate your idea and updated my question to better reflect what I'm actually doing. I'm looking forward to your thoughts :) – Venemo Apr 17 '14 at 20:03
  • Mostly it depends on the `Field` interface. If clients can only use methods present at interface level, again, you can just provide your document with a bunch of methods that forward (I'm making it simple) calls to internal fields. If your clients derive classes adding methods, than you can only expose the fields and according to your Document definition you can't use a reference but need a pointer: return fields[name].get(); Also, your return type is a plain Field* – Fulvio Esposito Apr 18 '14 at 18:07
  • What do you think about references or `weak_ptr` as others suggested? – Venemo Apr 18 '14 at 20:18
  • There's actually little point in returning a reference when you have a pointer. Indeed you must pay attention as map::operator[] inserts a new value if name isn't found, so it'll return a unique_ptr initialized with null, and if you try to dereference the unique_ptr to get a reference you'll get an access violation reading 0x00000000 (a segfault on unixes). Indeed references can't be null, they must be bound to something. weak_ptr works only if you have a shared_ptr owning Fields and that might or might not be the case. If Document is the sole owner of Fields, than unique_ptr is appropriate. – Fulvio Esposito Apr 19 '14 at 14:58
  • After much-much consideration, I think this is the best way to solve the problem I have at hand. Thank you. – Venemo Dec 14 '14 at 14:28
4

I usually return references to the data not the reference to the unique_ptr:

const Bar &getBar(std::string name) const {
    return *bars[name];
}

If you want to be able to return empty item you can return raw pointer (and nullptr in case of empty). Or even better you can use boost::optional (std::optional in C++14).

If there is possibility that the reference would survive longer than the owner (multithreaded environment) I use shared_ptr internally and return weak_ptr in access methods.

Johny
  • 1,947
  • 13
  • 23
  • You are right. It was voted out from the specification draft. But it was moved to separate technical specification. – Johny Apr 16 '14 at 14:53
  • 2
    I would love to have it available soon. So many use cases where it can improve code quality! – Danvil Apr 16 '14 at 14:58
  • @Danvil : See https://github.com/akrzemi1/Optional/ for a working implementation. – ildjarn Apr 17 '14 at 09:35
  • I don't see any benefit of `optional` over a raw pointer in this case. An *observing* raw pointer will basically achieve the same goal. – Avidan Borisov Apr 23 '14 at 11:48
1

I'd return a std::weak_ptr< Bar > (if C++11) or boost::weak_ptr (if not C++11). This makes it explicit that this is heap memory and doesn't risk dangling a reference to non existent memory (like Bar & does). It also makes ownership explicit.

Andy
  • 1,663
  • 10
  • 17
  • I think weak_ptr promises conversion to shared_ptr which isn't in scope of the question. – Jon Kalb Apr 16 '14 at 21:20
  • I don't think it does make ownership explicit. It says, "share this if you want to". It would be easy to accidentally share a `Field` with another `Document` and then you could have changes to one `Document` effecting another. – Chris Drew Apr 19 '14 at 12:11
1

I generally don't like handing references to internal members. What happens if another thread modifies it? Instead if I cant hand out a copy, I prefer a more functional approach. Something like this.

class Foo {
private:
    std::map<std::string, std::unique_ptr<Bar> > bars;

    // ...

public:

    // ...
    template<typename Callable> void withBar(const std::string& name, Callable call) const {
        //maybe a lock_guard here?
        auto iter = bars.find(name);
        if(iter != std::end(bars)) {
            call(iter->second.get());
        }
    }
}

This way the owned internal never "leaves" the class that owns it, and the owner can control invariants. Also can have niceties like making the code a noop if the requested entry doesn't exist. Can use it like,

myfoo.withBar("test", [] (const Bar* bar){
   bar->dostuff();
});
1

If possible, I would try to avoid exposing the internals of Document at all but failing that I would either return a const Field& or if you need it to be nullable a const Field*. Both clearly show that Document is retaining ownership. Also, make the method itself const.

You could have a non const version of the method that returns a Field& or Field* but I would avoid that if you can.

Chris Drew
  • 14,926
  • 3
  • 34
  • 54
0

This is not yet considered, I guess: return a proxy object with the same interface.

The holder of the proxy object has the ownership of that proxy-object. The proxy holds whatever reference(weak maybe) to the referenced object. When the object is deleted then you can raise some error. Th eproxy object does not have ownership.

Maybe it was already mentioned. I am not so familiar with C++.

User
  • 14,131
  • 2
  • 40
  • 59