Sure. If we made these changes RegionFactory::create would change to:
Region RegionFactory::create(const char*);
Since Region has a deleted copy constructor, we would have to implement a
move constructor and a move assignment operator.
Region(const Region&& other);
Region& operator=(const Region&& other);
Thus I think a call like following should not invoke copy constructor even
if compiler doesn't support copy-elision:
auto region = RegionFactory().create(...);
Thanks,
David
On Wed, Sep 6, 2017 at 2:31 PM, Jacob Barrett <jbarr...@pivotal.io> wrote:
> 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
> > > > >
> > > >
> > >
> >
>
