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
