+1 for the void return type
> On 14 Sep, 2017, at 13:39, Ernest Burghardt <eburgha...@pivotal.io> wrote:
>
> +1 cleans up the public API and code using this as you can see in the
> proposed changes on Jake's fork
>
> On Wed, Sep 13, 2017 at 3:30 PM, Jacob Barrett <jbarr...@pivotal.io> wrote:
>
>> I would like to propose a change:
>> Serializable* Serializable::formData(DataInput& in)
>> to
>> void Serializable::formData(DataInput& in)
>>
>> The current signature allows the object being deserialized to return an
>> object other than itself. The use of this trick is only done internally for
>> a few internal system types in what appears to have been an attempt to make
>> serialization more pluggable. The downside to this approach is that it
>> leaks this ability out to the public interface. Additionally concerning is
>> that the return type is a raw pointer to Serializable but typically the
>> object was created as a std::shared_ptr, which can lead to shard_ptr errors
>> if you don't property check and swap the returned raw pointer against the
>> original shared_ptr. Lastly the return value is a pointer to the most base
>> interface providing little utility and type safety.
>>
>> A couple of spikes investigated changing the signature to:
>> std::shared_ptr<Serializable> Serializable::formData(DataInput& in)
>> and:
>> template<class T>
>> std::shared_ptr<T> Serializable::formData(DataInput& in)
>> But both approaches left some dirty things laying around. First the
>> templated version just caused all sorts of pain and failed when the value
>> was replaced on the fromData. The more generic share_ptr<Serializable>
>> approach uncovered a plethora of places internally that we use the
>> Serializable::fromData to deserialize objects as parts of protocol messages
>> and used as internal data where they weren't originally created as
>> shared_ptrs, so the opposite problem to what we were trying to solve.
>>
>> The final spike investigated using void. In doing so we only had to make
>> small changes to the way PDX types were being deserialized. The void
>> signature is also more consistent with the Java DataSerializable interface.
>> By making it void the ambiguity of using or checking the return value goes
>> away.
>>
>> You can see the proposed changes on my fork at
>> https://github.com/pivotal-jbarrett/geode-native/tree/wip/fromDataVoid.
>>
>> Thoughts?
>>
>> -Jake
>>
