Great, can you provide some examples of what some of the APIs would look like if we made these changes?
On Wed, Sep 6, 2017 at 2:29 PM David Kimura <dkim...@pivotal.io> wrote: > Good point. In those cases, the copy constructor should have been deleted > to prevent the copy build up. For objects when copy constructor is deleted > *and* compiler doesn't perform RVO, I think we'd likely need to add a move > constructor. > > Thanks, > David > > On Wed, Sep 6, 2017 at 2:01 PM, Jacob Barrett <jbarr...@pivotal.io> wrote: > > > Bare object sounds nice to me. The caller has the option at that point to > > copy to heap into smart pointer or keep on stack. > > > > Mixed with the fluent/builder conversation in another thread this would > > simplify to: > > > > auto cache = CacheFactory().setSomething().create(); > > > > Which I think looks pretty darn readable. > > > > If they want to stash that cache in the heap and pass it around to other > > callers then: > > > > auto cache = std::shared_ptr<Cache>(CacheFactory().setSomething(). > > create()); > > > > Which isn't as nice to read but gets the job done. > > > > I think with many of these calls they are not frequent calls to if any of > > them resulted in copies it wouldn't be the end of the world. What we need > > to make sure doesn't happen though is that a copy builds up more of the > > underlying resources. I would not want a copy of Cache to returned by > > CacheFactory to result in another set of connections to the servers. > > > > Have you looked closely at what copy would do in the case of each of the > > objects we are looking at on the public API? Assuming no optimization > and a > > copy is performed then are there resources be re-allocated that we don't > > want re-alloacted. If that is the case then we need to consider > > alternatives like ref, ptr or pimpl. > > > > -Jake > > > > > > On Wed, Sep 6, 2017 at 12:53 PM Ernest Burghardt <eburgha...@pivotal.io> > > wrote: > > > > > std::unique_ptr looks like a good option. > > > > > > then again if the typical usage is like so: > > > > > > cachePtr = CacheFactory::createCacheFactory(pp)->create(); > > > > > > it seems simple enough to just return the bare object > > > > > > EB > > > > > > On Wed, Sep 6, 2017 at 1:27 PM, David Kimura <dkim...@pivotal.io> > wrote: > > > > > > > What type should C++ object creation functions return (e.g. pointer, > > > smart > > > > pointer, reference, etc.)? Several of our C++ API's return shared > > > > pointers. For example in the following function signature [1]: > > > > > > > > std::shared_ptr<CacheFactory> CacheFactory:: > > createCacheFactory(...); > > > > > > > > Here the only case I can see for shared pointer is to indicate > > ownership > > > of > > > > CacheFactory. Ideally this should probably be std::unique_ptr > because > > > > callee shouldn't share ownership. However, I don't see the point of > > > using > > > > a pointer at all.. I suggest we return the bare object, like the > > > following > > > > signature: > > > > > > > > CacheFactory CacheFactory::createCacheFactory(...); > > > > > > > > In C++03, this would have been a performance hit because we'd end up > > with > > > > an added call to the copy constructor. In C++11, std::unique_ptr > gives > > > > std::move for free and thus avoids copy-constructor call. However, > most > > > > modern C++11 compilers already perform copy-elision here. In fact, > > C++17 > > > > standard dictates that compilers must perform RVO here. Therefore it > > > > doesn't seem to me that std::shared_ptr or std::unique_ptr buys us > much > > > in > > > > this situation. > > > > > > > > Thoughts? > > > > > > > > Thanks, > > > > David > > > > > > > > > > > > [1] https://github.com/apache/geode-native/blob/develop/ > > > > cppcache/include/geode/CacheFactory.hpp#L54 > > > > > > > > > >
