Re: [DISCUSS] Streamline return value from RemoteQuery

[David Kimura]

On Wed, Aug 15, 2018 at 5:24 AM, Jacob Barrett  wrote:

> Oh wouldn’t it be nice if it was asynchronous... :(


"Hold my beer"  :P

Re: [DISCUSS] Streamline return value from RemoteQuery

[David Kimura]

If this is a non-blocking function (and maybe even if it isn't), then it
might be worth considering a future/promise contract.   Perhaps something
like folly which provides a very rich interface.

Thanks,
David

On Tue, Aug 14, 2018 at 12:57 PM, Jacob Barrett  wrote:

> If we are going to change it I wonder if we can’t do better or look for
> some other standard to adopt.
>
> For the C++ side could we adopt something that is maybe std::tuple or
> std::vector based? Tuple probably doesn’t work because it’s fixed at
> compile time but std::vector and StructSet are very similar in behavior.
>
> -Jake
>
>
> > On Aug 14, 2018, at 11:01 AM, Dan Smith  wrote:
> >
> > +1 for just having StructSet.
> >
> > Return types should always be strongly typed, the user shouldn't have to
> > introspect or guess based on their query what the return type actually
> will
> > be at runtime.
> >
> > If we think there is value in having separate ResultSet and StructSet
> > results, we could have a separate query::executeXXX method for each type.
> > But I think just returning StructSet seems reasonable.
> >
> > The Java API for query is even worse. I think the Java API actually
> returns
> > an Object, which the user has to cast into one of three things - an
> > individual value, SelectResult of values, or a SelectResults of structs.
> We
> > should fix that too!
> >
> > -Dan
> >
> > On Tue, Aug 14, 2018 at 10:47 AM, Anilkumar Gingade  >
> > wrote:
> >
> >> In Java, they are separated so that the results can be managed
> effectively.
> >> For example StructSet has its own implementation to manage the query
> >> results that are structs (more than one projection attributes).
> >>
> >> -Anil
> >>
> >>
> >>
> >>> On Tue, Aug 14, 2018 at 8:28 AM David Kimura 
> wrote:
> >>>
> >>> I have a couple questions:
> >>>
> >>> Do you have an idea or theories of what was the original intent behind
> >>> separating ResultSet and StructSet?
> >>>
> >>> Is execute a blocking or non-blocking call and does the interface have
> >> any
> >>> guarantee of that?
> >>>
> >>> Thanks,
> >>> David
> >>>
> >>> On Mon, Aug 13, 2018 at 4:06 PM, Ernest Burghardt <
> eburgha...@pivotal.io
> >>>
> >>> wrote:
> >>>
> >>>> Currently, geode-native's query::execute returns a
> >>>> shared_ptr and
> >>>> that pointer can be either ResultSet or StructSet.
> >>>>
> >>>>
> >>>> RemoteQuery::execute contains logical code to determine with
> >> QueryResults
> >>>> are greater than 0... We should look at removing this logic and only
> >>>> returning StructSets
> >>>> This allows removal of ResultSet which will streamline the API and
> >>>> associated code...
> >>>>
> >>>> This duality is unnecessary and should be removed.
> >>>> I am proposing that the results only return  StructSet
> >>>>
> >>>> Best,
> >>>> EB
> >>>>
> >>>
> >>
>

Re: [DISCUSS] Streamline return value from RemoteQuery

[David Kimura]

I have a couple questions:

Do you have an idea or theories of what was the original intent behind
separating ResultSet and StructSet?

Is execute a blocking or non-blocking call and does the interface have any
guarantee of that?

Thanks,
David

On Mon, Aug 13, 2018 at 4:06 PM, Ernest Burghardt 
wrote:

> Currently, geode-native's query::execute returns a
> shared_ptr and
> that pointer can be either ResultSet or StructSet.
>
>
> RemoteQuery::execute contains logical code to determine with QueryResults
> are greater than 0... We should look at removing this logic and only
> returning StructSets
> This allows removal of ResultSet which will streamline the API and
> associated code...
>
> This duality is unnecessary and should be removed.
> I am proposing that the results only return  StructSet
>
> Best,
> EB
>

Re: Permissions to update JIRA tickets

[David Kimura]

Thanks Dan!

On Thu, Feb 1, 2018 at 11:32 AM, Dan Smith <dsm...@pivotal.io> wrote:

> Hi David,
>
> Done - you should have access.
>
> Thanks!
> -Dan
>
> On Thu, Feb 1, 2018 at 11:14 AM, David Kimura <dkim...@pivotal.io> wrote:
>
> > Hi,
> >
> > I would like to be able to update the status of Geode JIRA tickets I
> worked
> > on.  Would someone be able to give me the permissions to do so?  My
> > username in JIRA is dkimura.
> >
> > Thanks,
> > David
> >
>

Permissions to update JIRA tickets

[David Kimura]

Hi,

I would like to be able to update the status of Geode JIRA tickets I worked
on.  Would someone be able to give me the permissions to do so?  My
username in JIRA is dkimura.

Thanks,
David

Re: Discussion: Native PDX Reader/Writer std::string

[David Kimura]

Given that C++17 is feature-complete, it seems prudent to adopt those
patterns as much as possible.

My fear with #4 is that we're introducing a special case API that users
become accustomed to and then unwilling to later switch to std::optional.
And then we'd end up supporting multiple ways to do the same thing.  And
even if we can delete the special case API after C++17, there's still API
churn for our users.  Because of that, I favor using optional even if we
have to roll our own.  And then after C++17 we change our implementation to
alias std::optional and there shouldn't be any code changes required from
user point of view.

Thanks,
David

On Tue, Dec 19, 2017 at 2:30 PM, Jacob Barrett  wrote:

> On Tue, Dec 19, 2017 at 1:52 PM Michael William Dodge 
> wrote:
>
> > Of those, #1 seems better than #2 and #3. Is there an elegant way to use
> a
> > sentinel value of std::string to represent null, e.g., static const
> > PdxReader::NULL_STRING = ?
> >
>
> I am absolutely opposed to sentinel values just from the standpoint that
> once you pick a value someone will want that value.
>
> The C++17 solution to this problem is std::optional, but
> unfortunately C++11 is our minimum. We could embrace boost::optional (for
> which C++17 std::optional derives), but this will cause clients to also
> depend on boost, which isn't ideal. We could roll our own optional...
>
> -Jake
>

Re: [Discussion] Geode-Native Removing Stats from Public API

[David Kimura]

I agree, a statistics interface seems beyond the scope of Geode Native
client responsibility.  Hiding or removing seems appropriate to me.

Thanks,
David

On Fri, Nov 17, 2017 at 11:29 AM, Ernest Burghardt
 wrote:
> +1 for removal
>
> On Thu, Nov 16, 2017 at 1:46 PM, Jacob Barrett  wrote:
>
>> I want to open a discussion regarding the removal of StatisticsFactory and
>> related APIs from the public API. I can't see that we would want the Geode
>> Native client to be a first class statistics/metrics gathering API. There
>> are plenty of other first class players in this space. If this isn't a
>> feature of the client then I suggest it be moved internally. It’s highly
>> unlikely it’s being used but in the case that it is we can consider moving
>> it back after some serious refactoring as it relies on an over abundance of
>> raw pointers. Rather than spend time refactoring it now let’s just hide it
>> away.
>>
>> -Jake
>>
>>
>>
>>

[Discussion] Native - Should C++ client support wchar_t type?

[David Kimura]

While working on removing out parameters, we noticed code that makes
assumption that wchar_t is always 16 bits.

virtual void PdxInstance::getField(const char* fieldName, wchar_t& value)
const = 0;

void PdxInstanceImpl::getField(const char* fieldname, wchar_t& value) const
{
auto dataInput = getDataInputForField(fieldname);
uint16_t temp = dataInput->readInt16();
value = static_cast(temp);
}



According to cppreference[1], this assumption is incorrect.  If that is the
case, should this implementation be fixed or can it be deleted altogether?

Thanks,
David

[1] http://en.cppreference.com/w/cpp/language/types

Re: [DISCUSS] C++ Returning null values

[David Kimura]

I'm not sure if this is the same as the sentinel value you mentioned, but
what about introducing a no-op region type and returning that?  I'm
thinking a null object pattern which would no-op and then nobody should
need to check if nullptr.

Thanks,
David

On Tue, Oct 10, 2017 at 10:27 AM, Jacob Barrett  wrote:

> Looking at a class like Region (Region.cpp) there are calls to get the
> parent region and sub regions, there are instances where a Region will not
> have a parent or subs. The current API returns shared_ptr that may be
> nullptr or a Region.
>
> Since we are trying to make an effort towards values over pointers should
> be considered some changes here? Obviously a reference is out of the
> question because it can't be null. A value is really out of the question
> too since it can't be null and making a sentinel value is not a great
> solution. Raw pointers are fine since they can be nullptr but make
> ownership ambiguous. Using shared_ptr is good since it can be nullptr and
> solves the ownership problem. Another option is to embrace the forthcoming
> std::optional available as boost::optional in C++11.
>
> I am leaning towards keeping it shared_ptr since using boost::optional
> would require users compile with boost. I don't think we should have
> anything on our API that is not ours of C++11. Requiring third party
> libraries to compile against our API doesn't fly right by me.
>
> Thoughts?
>

Re: [DISCUSS] Using out parameters and its effects on function overload resolution

[David Kimura]

I agree. I think returning tuple flows better and seems to be the way the
language is progressing.

Also, I think it's probably even more efficient than using out variable.
Using out variable we'd pay the cost of the default constructor and then
the cost populating any state which is essentially equivalent to a copy
constructor.  While returning tuples we can let the compiler perform RVO
and the standard library perform small object optimization.

The other question though is, could function overload resolution be a valid
use case for out variables (particularly in PdxReader, for example)?

Thanks,
David

On Mon, Oct 2, 2017 at 10:56 AM, Jacob Barrett <jbarr...@pivotal.io> wrote:

> Basic docs on the C++11 tuple use for multiple return types:
> http://en.cppreference.com/w/cpp/utility/tuple
>
> In C++11 we would have:
>
> std::tuple<int, std::string> foo::getAAndB() {...}
>
> And call it with:
>
> int a;
> std::string b;
> std::tie(a, b) = foo.getAAndB();
>
> While this isn't super pretty I would argue it is more readable than.
>
> std::string b;
> auto a = foo.getAAndB(b);
>
> I believe this is unclear when reading that b is an out variable rather
> than passing empty string value to the function.
>
> And certainly easier to understand than:
> int a;
> std::string b;
> foo.getAAndB(a, b);
>
> Again, is a and be inadvertently uninitialized?
>
> Alternatively the tuple can be called like:
>
> auto r = foo.getAAndB();
> auto a = std::get<0>(r);
> auto b = std::get<1>(r);
>
>
>
>
> Another interesting blog on the issue:
> https://dzone.com/articles/returning-multiple-values-from-functions-in-c
>
> -Jake
>
>
> On Mon, Oct 2, 2017 at 9:07 AM Jacob Barrett <jbarr...@pivotal.io> wrote:
>
> > We are already in contention with our style guide on many items. I
> suggest
> > we use it as a guideline only and establish our own rules. The more
> > research into the Google C++ guide is really driven by legacy C/C++
> issues
> > at Google.
> >
> > Regarding the in/out issue I am for multiple return types so that we
> > migrate to a more functional programming style. An interesting article I
> > read last week on this issue sheds some good light on why,
> > https://www.fluentcpp.com/2016/11/22/make-your-functions-functional/.
> >
> > As a bonus, once we go C++17 (in 2020ish I am sure) we can use auto
> > structured return values.
> > auto [a, b] = foo.getAAndB();
> >
> > -Jake
> >
> >
> > On Wed, Sep 27, 2017 at 1:14 PM David Kimura <dkim...@pivotal.io> wrote:
> >
> >> I forgot to mention, and it's probably worth noting, that passing
> >> non-const
> >> ref knowingly defies our style-guide.
> >>
> >> https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> >>
> >> Thanks,
> >> David
> >>
> >> On Wed, Sep 27, 2017 at 12:32 PM, Ernest Burghardt <
> eburgha...@pivotal.io
> >> >
> >> wrote:
> >>
> >> > Currently we have a mix of the counter argument...
> >> >
> >> > we use return values and you have to call the explicitly named
> methods:
> >> > double* readDoubleArray(const char* fieldName, int32_t& length)
> >> > int64_t* readLongArray(const char* fieldName, int32_t& length)
> >> >
> >> > I'm good with non-const refs in-out to the specific method calls OR
> >> > overload readArray and different return values...
> >> >
> >> > EB
> >> >
> >> > On Wed, Sep 27, 2017 at 1:27 PM, Michael William Dodge <
> >> mdo...@pivotal.io>
> >> > wrote:
> >> >
> >> > > I like the idea of using non-const references for in-out parameters
> >> only
> >> > > and using tuples for the cases where there are multiple out
> >> parameters.
> >> > > Yes, the return type does not participate in overload resolution
> but I
> >> > > don't think there would be many cases where that would be an issue.
> >> (But
> >> > I
> >> > > haven't done any research so that's just a WAG.)
> >> > >
> >> > > Sarge
> >> > >
> >> > > > On 27 Sep, 2017, at 12:22, David Kimura <dkim...@pivotal.io>
> wrote:
> >> > > >
> >> > > > Is there a use case in our C++ client code to ever use out
> >> parameters?
> >> > > >
> >> > > > Currently code uses out parameters to return multiple values from
&

[DISCUSS] Using out parameters and its effects on function overload resolution

[David Kimura]

Is there a use case in our C++ client code to ever use out parameters?

Currently code uses out parameters to return multiple values from a
function.  [1]

virtual int64_t* PdxReader::readLongArray(const char* fieldName, int32_t&
length) = 0;


In this case, we could return a tuple instead.  I think the advantage of
this is it doesn't force a separation between the variable declaration and
assignment, which may lead to spaghetti code.  I think avoiding out
parameters also leads to more well defined behavior.  What would one expect
if they called getCqListeners with an already partially populated vector?
[2]

virtual void CqAttributes::getCqListeners(std::vector

Re: [Vote] Better type checking by moving PdxWrapper from a standard class to a template class.

[David Kimura]

I briefly scanned some docs, but I'm still not sure I follow why this needs
to be in our domain.  It says PdxWrapper is "for domain objects that you
cannot or do not want to modify".

Why can't the application create an opaque wrapper around the object that
cannot be modified and register that wrapper with the serialization
registry - just like other serializable classes without us being the wiser
that it is a wrapper?

Thanks,
David

On Thu, Sep 21, 2017 at 11:49 AM, Jacob Barrett <jbarr...@pivotal.io> wrote:

> It may be work looking into the documentation a little to understand the
> purpose of the PdxWrapper.
>
> On Thu, Sep 21, 2017 at 11:37 AM David Kimura <dkim...@pivotal.io> wrote:
>
> > Is using PdxWrapper any different than if user added their type into the
> > serialization registry?  If not, then do we really want to provide 2 ways
> > to do the same thing?
> >
> > Thanks,
> > David
> >
> > On Thu, Sep 21, 2017 at 10:24 AM, Michael William Dodge <
> mdo...@pivotal.io
> > >
> > wrote:
> >
> > > +1 for type safety
> > >
> > > Sarge
> > >
> > > > On 21 Sep, 2017, at 10:21, Mark Hanson <mhan...@pivotal.io> wrote:
> > > >
> > > > Here is a link to my branch in my fork that has the changes on it.
> > > >
> > > > https://github.com/mhansonp/geode-native/tree/wip/templatePdxWrapper
> > > >
> > > > Thanks,
> > > > Mark
> > > >
> > > > On Thu, Sep 21, 2017 at 10:19 AM, Mark Hanson <mhan...@pivotal.io>
> > > wrote:
> > > >
> > > >> Hi All,
> > > >>
> > > >> In reviewing the PdxWrapper class it, it seemed like it would be a
> > good
> > > >> move to make this a template class. This will allow better type
> > checking
> > > >> anytime we use it.
> > > >>
> > > >> An example of what is being planned is to change from
> > > >>
> > > >> MyClass object;
> > > >> PdxWrapper((void *) object,...)
> > > >> to PdxWrapper(object, )
> > > >>
> > > >> I have a chunk of code moved over that seems to show it is possible,
> > > >> though there are some bugs that need to be addressed, so it is not a
> > > done
> > > >> deal.
> > > >>
> > > >> What do people think?
> > > >>
> > > >> Thanks,
> > > >> Mark
> > > >>
> > >
> > >
> >
>

Re: Return types form methods on Cache

[David Kimura]

Cool. +1 value model.

Thanks,
David

On Tue, Sep 19, 2017 at 11:15 AM, Jacob Barrett <jbarr...@pivotal.io> wrote:

> It isn’t. A Region should not outlive cache.
>
> > On Sep 19, 2017, at 10:54 AM, David Kimura <dkim...@pivotal.io> 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 <jbarr...@pivotal.io>
> 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 <dkim...@pivotal.io> 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 <dkim...@pivotal.io>
> >> 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 <jbarr...@pivotal.io>
> >>>> wrote:
> >>>>
> >>>>>
> >>>>>
> >>>>>> On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> 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

[David Kimura]

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 <jbarr...@pivotal.io> 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 <dkim...@pivotal.io> 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 <dkim...@pivotal.io>
> 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 <jbarr...@pivotal.io>
> >> wrote:
> >>
> >>>
> >>>
> >>>> On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> 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

[David Kimura]

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 <dkim...@pivotal.io> 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 <jbarr...@pivotal.io>
> wrote:
>
>>
>>
>> > On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> 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

[David Kimura]

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: [Discuss] Investigation of C++ object return types

[David Kimura]

Application code doesn't need to invoke move (and probably shouldn't).  In
the examples, I think it's more of an exercise to show what would happen if
invoked.  And really, I expect same badness to happen using shared pointer
method if we dereferenced a moved pointer.

Thanks,
David

On Mon, Sep 18, 2017 at 11:19 AM, Mark Hanson <mhan...@pivotal.io> wrote:

> If you think this is a significant ease of use benefit, why have to invoke
> move? I understand this code, but I have not seen it in recent memory.
> I agree that this may make use of a copy(move) constructor easier, but...
>
> If the consensus is that this is a significant ease of use benefit for the
> user, I am on board…
>
> +1
>
> Thanks,
> Mark
> > On Sep 18, 2017, at 11:15 AM, David Kimura <dkim...@pivotal.io> wrote:
> >
> > The benefit I see isn't for performance, it's flexibility and ease of use
> > by application developer.  Anything we can do to achieve this, I think
> is a
> > significant win.
> >
> > I think the reason for the various approaches is to determine whether we
> > can safely create the simple/flexible API without incurring a penalty on
> > performance.  Personally, I think the no nullptr benefit that Sarge
> > mentioned in previous thread is enough to warrant this approach serious
> > thought even though it provides no performance benefit.
> >
> > Thanks,
> > David
> >
> > On Mon, Sep 18, 2017 at 10:47 AM, Mark Hanson <mhan...@pivotal.io>
> wrote:
> >
> >> Hi All,
> >>
> >> As we are creating a user API, unless there is a significant performance
> >> benefit, I think we should try to take the simpler route.
> >>
> >> I see benefit to the approach being proposed, but I don’t see a
> >> significant benefit. Someone please school me if I am wrong.
> >>
> >> I am still in the shared pointer camp for cache and a raw pointer to
> cache
> >> in cacheImpl.
> >>
> >> Sorry, and thanks,
> >> Mark
> >>
> >>
> >>
> >>> On Sep 18, 2017, at 10:14 AM, David Kimura <dkim...@pivotal.io> wrote:
> >>>
> >>> +1 value approach (via some implementation from this thread)
> >>>
> >>> I think I like this.
> >>>
> >>> In all BAD cases, it's the user who shot themselves in the foot by
> using
> >>> std::move unsafely.  I expect this behavior is the same behavior as for
> >> any
> >>> other object.  And if we're ever able to get rid of the Cache/CacheImpl
> >>> circular dependency then we can add a copy constructor.
> >>>
> >>> I also like the idea of passing in a CacheConfig.  My concern though is
> >>> that it's piling on another big change.
> >>>
> >>> Thanks,
> >>> David
> >>>
> >>> On Sun, Sep 17, 2017 at 12:02 AM, Jacob Barrett <jbarr...@pivotal.io>
> >> wrote:
> >>>
> >>>> Ok, one more idea.
> >>>> https://gist.github.com/pivotal-jbarrett/
> beed5f70c1f3a238cef94832b13dab
> >> 7a
> >>>>
> >>>> The biggest issue with the value model is that we have been using a
> >> factory
> >>>> to build the Cache object. We really don't need one and if we get rid
> >> of it
> >>>> things look much better. They aren't perfect since we still need the
> >> back
> >>>> pointer from the impl to the cache for later use. If we didn't need
> that
> >>>> then we could allow copy construction. As it stands right now this
> >> version
> >>>> allows value, shared_prt, unique_ptr, etc. without any real overhead
> or
> >> RVO
> >>>> issues.
> >>>>
> >>>> The big change is that, rather than have a factory that we set a bunch
> >> of
> >>>> values on and then ask it to create the Cache, we create a CacheConfig
> >>>> object and pass that in to the Cache's constructor. Cache passes it to
> >>>> CacheImpl and CacheImpl sets itself up based on the config. If you
> look
> >> at
> >>>> what the current factory model does it isn't that different. For
> >> clarity I
> >>>> added an XmlCacheConfig object to that builds up the CacheConfig via
> >> Xml.
> >>>> You could imagine a YamlCacheConfig object *shiver*. The point is we
> >> don't
> >>>> care as long as we get a CacheConfig with all the things we support at
> >>

Re: [Discuss] Investigation of C++ object return types

[David Kimura]

The benefit I see isn't for performance, it's flexibility and ease of use
by application developer.  Anything we can do to achieve this, I think is a
significant win.

I think the reason for the various approaches is to determine whether we
can safely create the simple/flexible API without incurring a penalty on
performance.  Personally, I think the no nullptr benefit that Sarge
mentioned in previous thread is enough to warrant this approach serious
thought even though it provides no performance benefit.

Thanks,
David

On Mon, Sep 18, 2017 at 10:47 AM, Mark Hanson <mhan...@pivotal.io> wrote:

> Hi All,
>
> As we are creating a user API, unless there is a significant performance
> benefit, I think we should try to take the simpler route.
>
> I see benefit to the approach being proposed, but I don’t see a
> significant benefit. Someone please school me if I am wrong.
>
> I am still in the shared pointer camp for cache and a raw pointer to cache
> in cacheImpl.
>
> Sorry, and thanks,
> Mark
>
>
>
> > On Sep 18, 2017, at 10:14 AM, David Kimura <dkim...@pivotal.io> wrote:
> >
> > +1 value approach (via some implementation from this thread)
> >
> > I think I like this.
> >
> > In all BAD cases, it's the user who shot themselves in the foot by using
> > std::move unsafely.  I expect this behavior is the same behavior as for
> any
> > other object.  And if we're ever able to get rid of the Cache/CacheImpl
> > circular dependency then we can add a copy constructor.
> >
> > I also like the idea of passing in a CacheConfig.  My concern though is
> > that it's piling on another big change.
> >
> > Thanks,
> > David
> >
> > On Sun, Sep 17, 2017 at 12:02 AM, Jacob Barrett <jbarr...@pivotal.io>
> wrote:
> >
> >> Ok, one more idea.
> >> https://gist.github.com/pivotal-jbarrett/beed5f70c1f3a238cef94832b13dab
> 7a
> >>
> >> The biggest issue with the value model is that we have been using a
> factory
> >> to build the Cache object. We really don't need one and if we get rid
> of it
> >> things look much better. They aren't perfect since we still need the
> back
> >> pointer from the impl to the cache for later use. If we didn't need that
> >> then we could allow copy construction. As it stands right now this
> version
> >> allows value, shared_prt, unique_ptr, etc. without any real overhead or
> RVO
> >> issues.
> >>
> >> The big change is that, rather than have a factory that we set a bunch
> of
> >> values on and then ask it to create the Cache, we create a CacheConfig
> >> object and pass that in to the Cache's constructor. Cache passes it to
> >> CacheImpl and CacheImpl sets itself up based on the config. If you look
> at
> >> what the current factory model does it isn't that different. For
> clarity I
> >> added an XmlCacheConfig object to that builds up the CacheConfig via
> Xml.
> >> You could imagine a YamlCacheConfig object *shiver*. The point is we
> don't
> >> care as long as we get a CacheConfig with all the things we support at
> >> "init" time.
> >>
> >> I know it is a more radical change but I feel it is more C++ and more
> >> testable than the factory model. I also like that it solves some of the
> >> issues with the value model we were looking at.
> >>
> >> -Jake
> >>
> >> On Thu, Sep 14, 2017 at 5:16 PM Jacob Barrett <jbarr...@pivotal.io>
> wrote:
> >>
> >>> Y'all here is an attempt to get the best of both worlds.
> >>> https://gist.github.com/pivotal-jbarrett/
> 52ba9ec5de0b494368d1c5282ef188
> >> ef
> >>>
> >>> I thought I would try posting to Gist but so far not impressed, sorry.
> >>>
> >>> The Gist of it is that we can overcome the thirdpary or transient
> >>> reference back to the public Cache instance by keeping a reference to
> it
> >> in
> >>> the implementation instance and updating it whenever the move
> constructor
> >>> is called.
> >>>
> >>> The downside is if your run this test it doesn't show RVO kicking in on
> >>> the second test where we move the value into a shared pointer. There
> are
> >> a
> >>> couple of pitfalls you can stumble into as well by trying to used the
> >>> previous instance to access the cache after the move operation, as
> >>> illustrated by the "BAD" commented lines.
> >>>
> >>> The upside is the choices this gives 

Re: [Discuss] Investigation of C++ object return types

[David Kimura]

> >> > >
> >> > > On Thu, Sep 14, 2017 at 3:02 PM, Michael William Dodge <
> >> > mdo...@pivotal.io>
> >> > > wrote:
> >> > >
> >> > >> I generally dig reference-counted pointers for avoiding lifetime
> >> issues
> >> > >> with objects allocated off the heap but I can live with bare
> >> pointers,
> >> > too.
> >> > >>
> >> > >> Sarge
> >> > >>
> >> > >>> On 13 Sep, 2017, at 16:25, Mark Hanson <mhan...@pivotal.io>
> wrote:
> >> > >>>
> >> > >>> Hi All,
> >> > >>>
> >> > >>> I favor the “pointer" approach that is identified in the code
> >> sample.
> >> > >> There is greater clarity and less bytes seemingly created and
> >> written.
> >> > We
> >> > >> do sacrifice the potential ease of using an object, but in all, I
> >> think
> >> > the
> >> > >> way our code is structured. It is not conducive to do a value
> >> approach,
> >> > >> from an efficiency standpoint,  because of our use of the pimpl
> >> model.
> >> > >>>
> >> > >>> Thanks,
> >> > >>> Mark
> >> > >>>
> >> > >>>> On Sep 12, 2017, at 11:09 AM, Jacob Barrett <jbarr...@pivotal.io
> >
> >> > >> wrote:
> >> > >>>>
> >> > >>>> My biggest concern with this model is that access to the public
> >> Cache
> >> > >>>> object from other public objects results in additional
> allocations
> >> of
> >> > a
> >> > >>>> Cache value. Think about when we are inside a Serializable object
> >> and
> >> > we
> >> > >>>> access the Cache from DataOutput.
> >> > >>>>
> >> > >>>> As value:
> >> > >>>> Serializable* MyClass::fromData(DataInput& dataInput) {
> >> > >>>> auto cache = dataOutput.getCache();
> >> > >>>> ...
> >> > >>>> }
> >> > >>>> In this the value of cache will RVO the allocation of Cache in
> the
> >> > >> getCache
> >> > >>>> call into the stack of this method, great. The problem is that
> >> Cache
> >> > >> must
> >> > >>>> contain a std::shared_ptr which means that each
> >> allocation
> >> > >> is 8
> >> > >>>> bytes (pointer to control block and pointer to CacheImpl) as well
> >> as
> >> > >> having
> >> > >>>> to increment the strong counter in the control block. On
> >> exit/descope,
> >> > >> the
> >> > >>>> Cache will have to decrement the control block as well.
> >> > >>>>
> >> > >>>> Using current shared_ptr pimple model:
> >> > >>>> Serializable* MyClass::fromData(DataInput& dataInput) {
> >> > >>>> auto& cache = dataOutput.getCache();
> >> > >>>> ...
> >> > >>>> }
> >> > >>>> We only suffer the ref allocation of 4 bytes and no ref count.
> >> Since
> >> > >> this
> >> > >>>> function call can't survive past the lifespan of Cache/CacheImpl
> >> they
> >> > >> don't
> >> > >>>> need to have shared_ptr and refcounting.
> >> > >>>>
> >> > >>>> Given that this method could be called numerous times is the
> >> overhead
> >> > of
> >> > >>>> the value version going to be a significant performance issue?
> >> > >>>>
> >> > >>>> I worry that moves and RVO is just beyond most developers. Heck I
> >> > didn't
> >> > >>>> know anything about it until we started exploring it.
> >> > >>>>
> >> > >>>>
> >> > >>>>
> >> > >>>> -Jake
> >> > >>>>
> >> > >>>>
> >> > >>>> On Tue, Sep 12, 2017 at 8:06 AM David Kimura <dkim...@pivotal.io
> >
> >> > >> wrote:
> >> > >>>>
> >> > >>>>> Follow up of attached discussion after more investigation.  I
> >> created
> >> > >> an
> >> > >>>>> example of returning Cache as shared pointer versus raw value:
> >> > >>>>>
> >> > >>>>>  https://github.com/dgkimura/geode-native-sandbox
> >> > >>>>>
> >> > >>>>> I still like returning by value as it lets the user do what they
> >> want
> >> > >> with
> >> > >>>>> their object.
> >> > >>>>>
> >> > >>>>>  // Here user creates object on their stack.
> >> > >>>>>  auto c = CacheFactory::createFactory().create();
> >> > >>>>>
> >> > >>>>>  // Here user creates smart pointer in their heap.
> >> > >>>>>  auto cptr =
> >> > >>>>> std::make_shared(CacheFactory::createFactory().
> create());
> >> > >>>>>
> >> > >>>>> Difficulty of implementing this is high due to circular
> >> dependencies
> >> > of
> >> > >>>>> Cache/CacheImpl as well as objects hanging off CacheImpl that
> >> return
> >> > >>>>> Cache.  We must be extra careful when dealing with move/copy
> >> > semantics
> >> > >> of
> >> > >>>>> Cache/CacheImpl.
> >> > >>>>>
> >> > >>>>> Alternative, is to keep as is and only permit heap allocations
> >> from
> >> > >>>>> factory using shared pointers.
> >> > >>>>>
> >> > >>>>> Thanks,
> >> > >>>>> David
> >> > >>>>>
> >> > >>>
> >> > >>
> >> > >>
> >> >
> >> >
> >>
> >
>

Re: [VOTE] Moving away from virtual CacheableStringPtr toString() for Serializable objects in debug and logging to std::string and std::wstring

[David Kimura]

Actually, it looks like Jake is right.  According to documentation it's
undefined behavior since it's not a template specialization.

http://en.cppreference.com/w/cpp/language/extending_std

Thanks,
David

On Fri, Sep 15, 2017 at 10:37 AM, Ernest Burghardt 
wrote:

> cool that we could export into the std namespace, but as a library I don't
> think it is a good idea as a user of our library might have done the same
> thing and that is a situation we can easily/pro-actively avoid
>
> +1 object::toString()
>
> On Fri, Sep 15, 2017 at 11:33 AM, Mark Hanson  wrote:
>
> > Comments inline….
> > > On Sep 15, 2017, at 10:25 AM, Jacob Barrett 
> wrote:
> > >
> > > -1 to all, comments below.
> > >
> > >
> > >> On Sep 15, 2017, at 10:18 AM, Mark Hanson  wrote:
> > >>
> > >> So we have two approaches as their has been a veto on w_string, so it
> > will not be used.
> > >
> > > Who said anything about veto? Nobody has veto authority. I simply
> stated
> > and opinion on the use of wstring based on my research in the past.
> > >
> >
> > Ok, not vetoed.
> >
> > >> Approach 1) std::string str = object.to_string();
> > > To remain consistent with the APIs use of camel case this should be
> > toString();
> > >
> >
> > Uh, yeah. Concept not code...
> >
> > >
> > >> Approach 2) std::string str = std::to_string(object);
> > >
> > > Nobody countered my assertion that this is not legal to export into std
> > names space.
> > >
> >
> > class blobject
> > {
> >   bool blobBool;
> > };
> >
> >
> > namespace std {
> >   std::string to_String(blobject * blob)
> >   {
> > return std::string("works");
> >   }
> > }
> >
> >
> > int main() {
> >
> >   blobject * blob = new blobject();
> >   std::string str = std::to_String(blob);
> >   std::cout << "Hello, World! " <<  "value = " << str << std::endl;
> >   return 0;
> > }
> > /Users/mhanson/CLionProjects/untitled1/cmake-build-debug/untitled1
> > Hello, World! value = works
> >
> > Process finished with exit code 0
> >
> >
> > > -Jake
> > >
> > >
> >
> >
>

Re: [Discuss] Moving away from virtual CacheableStringPtr toString() for Serializable objects in debug and logging to std::string and std::wstring

[David Kimura]

Seems like a good idea.

Here's a quick thought - I think c++11 is not really an obj.toString() kind
of language (like Java or C#).  Would it make sense to add std::to_string()
and std::to_wstring() equivalents for CacheableString?  This might mean
implementing following functions:

std::string to_string(CacheableString value);
std::wstring to_wstring(CacheableString value);

So that we can do something like:

std::cout << std::to_wstring(pdxser);

Thanks,
David

On Thu, Sep 14, 2017 at 11:10 AM, Michael William Dodge 
wrote:

> +1 for std::string and std::wstring.
>
> Sarge
>
> > On 14 Sep, 2017, at 11:10, Mark Hanson  wrote:
> >
> > Hi All,
> >
> > I wanted to broach the subject of moving away from moving away from
> CacheableStringPtrs for the toString representation of Serializable. It
> would seem desirable to move to std::string and std::wstring to use more
> basic types that would be faster to log and the code would be simpler for a
> user.
> >
> > Are there any opinions on this subject?
> >
> > Here is a before and after look at a chunk of code
> >
> > Before
> >
> > CacheableStringPtr ptr = pdxser->toString();
> > if (ptr->isWideString()) {
> >  printf(" query idx %d pulled object %S  :: \n", i,
> > ptr->asWChar());
> > } else {
> >  printf(" query idx %d pulled object %s  :: \n", i,
> > ptr->asChar());
> > }
> >
> > After
> >
> >
> > if (pdxser->isWideString()) {
> >   std::cout << " query idx “ << i << "pulled object ” <<
> pdxser->toWString() << std::endl;
> > } else {
> >   std::cout << " query idx “ << i << "pulled object ” <<
> pdxser->toString() << std::endl;
> > }
> >
> >
> > Thanks,
> > Mark
>
>

[Discuss] Investigation of C++ object return types

[David Kimura]

Follow up of attached discussion after more investigation.  I created an
example of returning Cache as shared pointer versus raw value:

https://github.com/dgkimura/geode-native-sandbox

I still like returning by value as it lets the user do what they want with
their object.

// Here user creates object on their stack.
auto c = CacheFactory::createFactory().create();

// Here user creates smart pointer in their heap.
auto cptr =
std::make_shared(CacheFactory::createFactory().create());

Difficulty of implementing this is high due to circular dependencies of
Cache/CacheImpl as well as objects hanging off CacheImpl that return
Cache.  We must be extra careful when dealing with move/copy semantics of
Cache/CacheImpl.

Alternative, is to keep as is and only permit heap allocations from factory
using shared pointers.

Thanks,
David

Re: [Discuss] What type should C++ object creation functions return?

[David Kimura]

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(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::
> > > 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
> > > > >
> > > >
> > >
> >
>

Re: [Discuss] What type should C++ object creation functions return?

[David Kimura]

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(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::
> 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
> > >
> >
>

[DISCUSS] geode-native c++ exceptions

[David Kimura]

C++ client uses a custom standalone base exception class [1
]
and several derived exceptions [2

].

Using exceptions seems contrary to the Google C++ Style Guide we adopted,
which states: *"do not use C++ exceptions"* [3
].  Here is a
link [4 ] to a cppcon
talk defending their decision.  Does it make sense to enforce this rule on
our code base?

If we decide to knowingly ignore this rule, would we want to continue to
propagate custom exceptions or switch to standard exceptions?  At the very
least, it seems to me that our custom exceptions should derive from their
most closely matching standard exception counterparts.  Thoughts?

Thanks,
David

[1] https://github.com/apache/geode-native/blob/develop/cppcache
/include/geode/Exception.hpp#L45-L123
[2] https://github.com/apache/geode-native/blob/develop/cppcache
/include/geode/ExceptionTypes.hpp#L70-L393
[3] https://google.github.io/styleguide/cppguide.html#Exceptions
[4] https://www.youtube.com/watch?v=NOCElcMcFik#t=30m29s

Re: [jira] [Commented] (GEODE-2441) Remove PDXAutoSerializer

[David Kimura]

It has been moved into a contrib directory outside the core sources where
other standalone tools can live too.

https://github.com/apache/geode-native/tree/develop/contrib/pdxautoserializer

Thanks,
David

On Wed, Mar 1, 2017 at 9:28 AM, Michael Stolz  wrote:

> We should make sure to find a new home for the PDXAutoSerializer, not just
> remove it.
> It is an important part of the product.
> Without it, PDX can't claim C++ compatibility.
> I assume a separate Jira has bin opened for that work?
>
>
> --
> Mike Stolz
> Principal Engineer, GemFire Product Manager
> Mobile: +1-631-835-4771
>
> On Wed, Mar 1, 2017 at 10:41 AM, ASF GitHub Bot (JIRA) 
> wrote:
>
> >
> > [ https://issues.apache.org/jira/browse/GEODE-2441?page=
> > com.atlassian.jira.plugin.system.issuetabpanels:comment-
> > tabpanel=15890406#comment-15890406 ]
> >
> > ASF GitHub Bot commented on GEODE-2441:
> > ---
> >
> > Github user echobravopapa commented on a diff in the pull request:
> >
> > https://github.com/apache/geode-native/pull/42#discussion_r103713640
> >
> > --- Diff: src/quickstart/cpp/CMakeLists.txt ---
> > @@ -26,16 +26,6 @@ elseif(UNIX)
> >  set(DYNAMIC_LIBRARY_PATH LD_LIBRARY_PATH=${
> NATIVECLIENT_DIR}/lib)
> >  endif()
> >
> > --- End diff --
> >
> > -verbose flag LTGM, I had made the same changes locally
> >
> >
> > > Remove PDXAutoSerializer
> > > -
> > >
> > > Key: GEODE-2441
> > > URL: https://issues.apache.org/jira/browse/GEODE-2441
> > > Project: Geode
> > >  Issue Type: Bug
> > >  Components: native client
> > >Reporter: Ernest Burghardt
> > >
> > > Remove PDXAutoSerializer utility that generates PDX serialization C++
> > source that you can include in your project to (de)serialize your C++
> > classes.
> >
> >
> >
> > --
> > This message was sent by Atlassian JIRA
> > (v6.3.15#6346)
> >
>

[jira] [Commented] (GEODE-2441) Remove PDXAutoSerializer

[David Kimura (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15876425#comment-15876425
 ] 

David Kimura commented on GEODE-2441:
-

Current idea is to move PDXAutoSerializer either to geode-examples repo or into 
a utils directory (outside of normal build process) in geode-native.  Purpose 
of this JIRA is to reduce dependencies in the build process and extract 
non-core pieces of code from source tree.  In doing this, we hope that by 
reducing build complexity and core code size, the code will be easier to dive 
into and maintain.

> Remove PDXAutoSerializer 
> -
>
> Key: GEODE-2441
> URL: https://issues.apache.org/jira/browse/GEODE-2441
> Project: Geode
>  Issue Type: Bug
>  Components: native client
>Reporter: Ernest Burghardt
>
> Remove PDXAutoSerializer utility that generates PDX serialization C++ source 
> that you can include in your project to (de)serialize your C++ classes.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Created] (GEODE-2484) Remove ACE from native client dependencies

[David Kimura (JIRA)]

David Kimura created GEODE-2484:
---

 Summary: Remove ACE from native client dependencies
 Key: GEODE-2484
 URL: https://issues.apache.org/jira/browse/GEODE-2484
 Project: Geode
  Issue Type: Task
  Components: native client
Reporter: David Kimura


Remove ACE from native client dependencies.

Replace ACE usage with C++11 and/or Boost 1.63+



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Re: New Repo for Native Client

[David Kimura]

I agree with Sarge.  I support using the standard tools for each language.
I'm inclined to think unification on a single tool can hurt more (e.g.
remember ant).  And if we don't draw the line in the build process then
where will we draw it?  I would hate to see more Java code reformatted into
C++ code.  I think there's a distinction between the languages for a reason.

Thanks,
David

On Tue, Jan 17, 2017 at 10:09 AM, Michael William Dodge 
wrote:

> To me this seems related to the question of the degree to which the Geode
> community should be homogeneous. Who are we trying to attract with the
> non-Java clients: the existing, Java-centric community or new community
> members from other platforms? If the former, gradle makes it easy for them
> to adopt the native client. If the latter, gradle is a barrier to entry.
>
> Sarge
>
> > On 17 Jan, 2017, at 09:34, Roman Shaposhnik 
> wrote:
> >
> > On Mon, Jan 16, 2017 at 8:47 PM, Jacob Barrett 
> wrote:
> >> Roman,
> >>
> >> I understand what you are saying. I think that since the build process
> >> between the Java Geode bits and the Native Geode bits will completely
> >> different it might help to have the separate. Until someone comes up
> with a
> >> good cross platform and cross language build tool that is commonly used
> in
> >> the development environments for each language these builds will remain
> >> different. Gradle sucks for building C++ and .NET sources and CMake
> sucks
> >> for building Java sources. Gradle is not popular in the native project
> >> world nor is CMake popular in the Java world.
> >
> > Personally I feel that standardizing on Gradle in 2017 would make sense
> > for C++ as well. Now, I hear what you're saying -- the popularity is an
> > issue, but that's only a problem for complex builds (at the level of the
> client)
> > which our client is not. At least not really.
> >
> > That said -- this comes back to my original point of thinking about
> contributor
> > community -- I could totally see folks who would otherwise have
> contributed
> > to the unification effort not liking the switch to Gradle. So yeah --
> > I hear you,
> > but personally I'd err on the side of unification since there are
> > greater benefits
> > to be reaped.
> >
> >> So making one build system to
> >> cover them all would just hurt everyone. Since the experience will be
> >> unique for each I feel that it justifies a separate repo but I can
> totally
> >> see the other side of just keeping it all together.
> >
> > FWIW: I think it will only hurt folks with preconceived notion of what a
> C++
> > build should look like. In my life, I've seen way too many different
> > builds systems
> > come and go to worry about that ;-)
> >
> > Thanks,
> > Roman
>
>