geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Barrett <jbarr...@pivotal.io>
Subject Re: [Discuss] What type should C++ object creation functions return?
Date Wed, 06 Sep 2017 21:31:13 GMT
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 <dkimura@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 <jbarrett@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 <eburghardt@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 <dkimura@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
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message