Re: Return types form methods on Cache
Cool. +1 value model. Thanks, David On Tue, Sep 19, 2017 at 11:15 AM, Jacob Barrettwrote: > It isn’t. A Region should not outlive cache. > > > On Sep 19, 2017, at 10:54 AM, David Kimura wrote: > > > > Oh, man. I hope I didn't just kill the original idea. Unless we are > > somehow using shared_from_this, it shouldn't be a divergence from > existing > > behavior? > > > > Thanks, > > David > > > >> On Tue, Sep 19, 2017 at 10:47 AM, Jacob Barrett > wrote: > >> > >> Region returned by this would no longer be valid. It’s “references” to > >> interns cache/region would be invalid. The behavior is undefined. > >> > >>> On Sep 19, 2017, at 10:24 AM, David Kimura wrote: > >>> > >>> Err... I missed a return in example above. > >>> > >>> Region r = [](){ return CacheFactory::create().getRegion("myregion"); > } > >> (); > >>> > On Tue, Sep 19, 2017 at 10:19 AM, David Kimura > >> wrote: > > I favor values, but we should probably be diligent. > > Do any of the objects returned from Cache depend on Cache to out-live > >> the > returned object? A quick scan suggested no, but we still have a > std::enable_shared_from_this. Maybe dead code. In code > >> example, > if this happens (for any cache.get*), could we be in trouble or is > this > user error? > > Region r = [](){ CacheFactory::create().getRegion("myregion"); }(); > // Cache is out of scope. > // What is expected behavior? > r.put("key", "value"); > > Thanks, > David > > On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett > wrote: > > > > > > >> On Sep 18, 2017, at 7:34 PM, Kirk Lund wrote: > >> > >> I would vote +1 for a more attractive, professional and > user-friendly > > API. > >> I'm not sure if there's a perf or memory-usage reason for using > >> "std::shared_ptr" to types instead of returning values, but the > end > >> result does not look like a professional API to me. > > > > There really isn’t, especially if you look at what we did dirty > > CacheFactory::getCache by returning a value that can be moved into > the > >> heap > > and a shared point of one wants but not being forced into it. RVO > >> tricks > > can event make that move a noop. > > > > auto r = cache.getRegion(...); > > Where decltype(r) == Region > > and > > auto rp = std::make_shared(cache->getRegion()); > > Where decltype(rp) == shared_ptr > > > > Would both be valid. > > > > > > > > >> >
Re: Return types form methods on Cache
It isn’t. A Region should not outlive cache. > On Sep 19, 2017, at 10:54 AM, David Kimurawrote: > > Oh, man. I hope I didn't just kill the original idea. Unless we are > somehow using shared_from_this, it shouldn't be a divergence from existing > behavior? > > Thanks, > David > >> On Tue, Sep 19, 2017 at 10:47 AM, Jacob Barrett wrote: >> >> Region returned by this would no longer be valid. It’s “references” to >> interns cache/region would be invalid. The behavior is undefined. >> >>> On Sep 19, 2017, at 10:24 AM, David Kimura wrote: >>> >>> Err... I missed a return in example above. >>> >>> Region r = [](){ return CacheFactory::create().getRegion("myregion"); } >> (); >>> On Tue, Sep 19, 2017 at 10:19 AM, David Kimura >> wrote: I favor values, but we should probably be diligent. Do any of the objects returned from Cache depend on Cache to out-live >> the returned object? A quick scan suggested no, but we still have a std::enable_shared_from_this. Maybe dead code. In code >> example, if this happens (for any cache.get*), could we be in trouble or is this user error? Region r = [](){ CacheFactory::create().getRegion("myregion"); }(); // Cache is out of scope. // What is expected behavior? r.put("key", "value"); Thanks, David On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett wrote: > > >> On Sep 18, 2017, at 7:34 PM, Kirk Lund wrote: >> >> I would vote +1 for a more attractive, professional and user-friendly > API. >> I'm not sure if there's a perf or memory-usage reason for using >> "std::shared_ptr" to types instead of returning values, but the end >> result does not look like a professional API to me. > > There really isn’t, especially if you look at what we did dirty > CacheFactory::getCache by returning a value that can be moved into the >> heap > and a shared point of one wants but not being forced into it. RVO >> tricks > can event make that move a noop. > > auto r = cache.getRegion(...); > Where decltype(r) == Region > and > auto rp = std::make_shared(cache->getRegion()); > Where decltype(rp) == shared_ptr > > Would both be valid. > > > >>
Re: Return types form methods on Cache
Oh, man. I hope I didn't just kill the original idea. Unless we are somehow using shared_from_this, it shouldn't be a divergence from existing behavior? Thanks, David On Tue, Sep 19, 2017 at 10:47 AM, Jacob Barrettwrote: > Region returned by this would no longer be valid. It’s “references” to > interns cache/region would be invalid. The behavior is undefined. > > > On Sep 19, 2017, at 10:24 AM, David Kimura wrote: > > > > Err... I missed a return in example above. > > > > Region r = [](){ return CacheFactory::create().getRegion("myregion"); } > (); > > > >> On Tue, Sep 19, 2017 at 10:19 AM, David Kimura > wrote: > >> > >> I favor values, but we should probably be diligent. > >> > >> Do any of the objects returned from Cache depend on Cache to out-live > the > >> returned object? A quick scan suggested no, but we still have a > >> std::enable_shared_from_this. Maybe dead code. In code > example, > >> if this happens (for any cache.get*), could we be in trouble or is this > >> user error? > >> > >> Region r = [](){ CacheFactory::create().getRegion("myregion"); }(); > >> // Cache is out of scope. > >> // What is expected behavior? > >> r.put("key", "value"); > >> > >> Thanks, > >> David > >> > >> On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett > >> wrote: > >> > >>> > >>> > On Sep 18, 2017, at 7:34 PM, Kirk Lund wrote: > > I would vote +1 for a more attractive, professional and user-friendly > >>> API. > I'm not sure if there's a perf or memory-usage reason for using > "std::shared_ptr" to types instead of returning values, but the end > result does not look like a professional API to me. > >>> > >>> There really isn’t, especially if you look at what we did dirty > >>> CacheFactory::getCache by returning a value that can be moved into the > heap > >>> and a shared point of one wants but not being forced into it. RVO > tricks > >>> can event make that move a noop. > >>> > >>> auto r = cache.getRegion(...); > >>> Where decltype(r) == Region > >>> and > >>> auto rp = std::make_shared(cache->getRegion()); > >>> Where decltype(rp) == shared_ptr > >>> > >>> Would both be valid. > >>> > >>> > >>> > >> >
Re: Return types form methods on Cache
Region returned by this would no longer be valid. It’s “references” to interns cache/region would be invalid. The behavior is undefined. > On Sep 19, 2017, at 10:24 AM, David Kimurawrote: > > Err... I missed a return in example above. > > Region r = [](){ return CacheFactory::create().getRegion("myregion"); } (); > >> On Tue, Sep 19, 2017 at 10:19 AM, David Kimura wrote: >> >> I favor values, but we should probably be diligent. >> >> Do any of the objects returned from Cache depend on Cache to out-live the >> returned object? A quick scan suggested no, but we still have a >> std::enable_shared_from_this. Maybe dead code. In code example, >> if this happens (for any cache.get*), could we be in trouble or is this >> user error? >> >> Region r = [](){ CacheFactory::create().getRegion("myregion"); }(); >> // Cache is out of scope. >> // What is expected behavior? >> r.put("key", "value"); >> >> Thanks, >> David >> >> On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett >> wrote: >> >>> >>> On Sep 18, 2017, at 7:34 PM, Kirk Lund wrote: I would vote +1 for a more attractive, professional and user-friendly >>> API. I'm not sure if there's a perf or memory-usage reason for using "std::shared_ptr" to types instead of returning values, but the end result does not look like a professional API to me. >>> >>> There really isn’t, especially if you look at what we did dirty >>> CacheFactory::getCache by returning a value that can be moved into the heap >>> and a shared point of one wants but not being forced into it. RVO tricks >>> can event make that move a noop. >>> >>> auto r = cache.getRegion(...); >>> Where decltype(r) == Region >>> and >>> auto rp = std::make_shared(cache->getRegion()); >>> Where decltype(rp) == shared_ptr >>> >>> Would both be valid. >>> >>> >>> >>
Re: Return types form methods on Cache
Err... I missed a return in example above. Region r = [](){ return CacheFactory::create().getRegion("myregion"); } (); On Tue, Sep 19, 2017 at 10:19 AM, David Kimurawrote: > I favor values, but we should probably be diligent. > > Do any of the objects returned from Cache depend on Cache to out-live the > returned object? A quick scan suggested no, but we still have a > std::enable_shared_from_this. Maybe dead code. In code example, > if this happens (for any cache.get*), could we be in trouble or is this > user error? > > Region r = [](){ CacheFactory::create().getRegion("myregion"); }(); > // Cache is out of scope. > // What is expected behavior? > r.put("key", "value"); > > Thanks, > David > > On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett > wrote: > >> >> >> > On Sep 18, 2017, at 7:34 PM, Kirk Lund wrote: >> > >> > I would vote +1 for a more attractive, professional and user-friendly >> API. >> > I'm not sure if there's a perf or memory-usage reason for using >> > "std::shared_ptr" to types instead of returning values, but the end >> > result does not look like a professional API to me. >> >> There really isn’t, especially if you look at what we did dirty >> CacheFactory::getCache by returning a value that can be moved into the heap >> and a shared point of one wants but not being forced into it. RVO tricks >> can event make that move a noop. >> >> auto r = cache.getRegion(...); >> Where decltype(r) == Region >> and >> auto rp = std::make_shared(cache->getRegion()); >> Where decltype(rp) == shared_ptr >> >> Would both be valid. >> >> >> >
Re: Return types form methods on Cache
I favor values, but we should probably be diligent. Do any of the objects returned from Cache depend on Cache to out-live the returned object? A quick scan suggested no, but we still have a std::enable_shared_from_this. Maybe dead code. In code example, if this happens (for any cache.get*), could we be in trouble or is this user error? Region r = [](){ CacheFactory::create().getRegion("myregion"); }(); // Cache is out of scope. // What is expected behavior? r.put("key", "value"); Thanks, David On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrettwrote: > > > > On Sep 18, 2017, at 7:34 PM, Kirk Lund wrote: > > > > I would vote +1 for a more attractive, professional and user-friendly > API. > > I'm not sure if there's a perf or memory-usage reason for using > > "std::shared_ptr" to types instead of returning values, but the end > > result does not look like a professional API to me. > > There really isn’t, especially if you look at what we did dirty > CacheFactory::getCache by returning a value that can be moved into the heap > and a shared point of one wants but not being forced into it. RVO tricks > can event make that move a noop. > > auto r = cache.getRegion(...); > Where decltype(r) == Region > and > auto rp = std::make_shared(cache->getRegion()); > Where decltype(rp) == shared_ptr > > Would both be valid. > > >
Re: Return types form methods on Cache
> On Sep 18, 2017, at 7:34 PM, Kirk Lundwrote: > > I would vote +1 for a more attractive, professional and user-friendly API. > I'm not sure if there's a perf or memory-usage reason for using > "std::shared_ptr" to types instead of returning values, but the end > result does not look like a professional API to me. There really isn’t, especially if you look at what we did dirty CacheFactory::getCache by returning a value that can be moved into the heap and a shared point of one wants but not being forced into it. RVO tricks can event make that move a noop. auto r = cache.getRegion(...); Where decltype(r) == Region and auto rp = std::make_shared(cache->getRegion()); Where decltype(rp) == shared_ptr Would both be valid.
Re: Return types form methods on Cache
I would vote +1 for a more attractive, professional and user-friendly API. I'm not sure if there's a perf or memory-usage reason for using "std::shared_ptr" to types instead of returning values, but the end result does not look like a professional API to me. I would favor a more user-friendly API. If there's a performance or memory constraint to returning value instead of shared ptr then applying such a pattern to getRegion is overkill at the wrong level of abstraction -- the user can easily keep the return value in a var instead of repeating the Cache::getRegion call. On Mon, Sep 18, 2017 at 3:23 PM, Jacob Barrettwrote: > So, now that we have selected to use the value model for > CacheFactory::create()/Cache, we need to look at what the methods on Cache > return. > > Starting with Cache::createRegionFactory method which returns > std::shared_ptr. I think it is very clear that this should > be a value type. > > RegionFactory Cache::createRegionFactory(...) const; > > > Looking then at Cache::getRegion things are more interesting. It currently > returns std::shared_ptr. Internally we keep a set of these > shared_ptrs in a map. Does it make sense to keep returning these > shared_ptrs or should we again switch to a value model? It would be great > for consistency on the public API to go the value model way even if > initially the value simply holds a shared_ptr to the internal region impl. > > Thoughts? >
Return types form methods on Cache
So, now that we have selected to use the value model for CacheFactory::create()/Cache, we need to look at what the methods on Cache return. Starting with Cache::createRegionFactory method which returns std::shared_ptr. I think it is very clear that this should be a value type. RegionFactory Cache::createRegionFactory(...) const; Looking then at Cache::getRegion things are more interesting. It currently returns std::shared_ptr. Internally we keep a set of these shared_ptrs in a map. Does it make sense to keep returning these shared_ptrs or should we again switch to a value model? It would be great for consistency on the public API to go the value model way even if initially the value simply holds a shared_ptr to the internal region impl. Thoughts?