+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 >
