Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented cloning_ptr #202

Merged
merged 5 commits into from
Sep 5, 2024
Merged

Conversation

JohanMabille
Copy link
Collaborator

Fixes #195

This is a very basic implementation that can be improved in the future:

  • add a deleter template parameter (similar to std::unique_ptr)
  • add a cloner template parameter with a default value. This sould allow to merge the cloning_ptr and the value_ptr classes.

Notice that this adds some complexity to the constructors that I did not want to handle at the moment. I'll open an issue when this is merged.

Copy link
Collaborator

@Klaim Klaim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some clarifications would help, but other than that LGTM.
We also had a discussion on the design I'll comment below.

* @tparam T The type to check
*/
template <class T>
concept clonable = requires(const T* t)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you dont need a pointer here, you could use a reference too.

include/sparrow_v01/utils/memory.hpp Outdated Show resolved Hide resolved
include/sparrow_v01/utils/memory.hpp Show resolved Hide resolved
include/sparrow_v01/utils/memory.hpp Outdated Show resolved Hide resolved
include/sparrow_v01/utils/memory.hpp Outdated Show resolved Hide resolved
@Klaim
Copy link
Collaborator

Klaim commented Sep 5, 2024

Design discussed privately and summarized below:

While this is enough for our usage, this is not how such type is usually designed when it is designed for general use.
For our current use, this is good enough, although this note is for the hypothetical future merge with value_ptr or re-design as more generic type.

The current design of clone_ptr relies on imposing to the type hierarchy it will be used with to provide a clone() function. But semantically speaking, this is strictly the same thing as a copy operator. Type hierarchies dont necessary have to prevent copies, the main issue with allowing copies with these is that the copy operations can't be virtual and therefore when you have, say a unique_ptr<A> with A being a virtual base type, and we want to copy whatever is pointed by that unique_ptr<A>, we can't without already knowing that specific type. This clone_ptr worksaround the problem by requiring the clone() function to benefit from virtual dispatch and therefore each inheriting type will do the right copy implementation, although it's still a copy.

The more generic way to think about it is to assume that if a type hierarchy provides copy operations, we can workaround the lack of virtual dispatch in these operations by wrapping these types with a type that does the virtual dispatch: basically, generating automatically the cloning function as a virtual function, based on the standard copy operations of the type being stored. This is more in line with about everything in generic computing, which relies on value semantics and the idea of "regular" (or "semi-regular") types - which is what makes standard algorithms works about everywhere.
The main advantage of such case is that it doesn't force the type hierarchy to implement a function very specific clone(), which is kind of intrusive for users, but it does require these types to be copyable, which is more acceptable in general and very common. The other advantage is that it can be implemented in various ways with various tradeoffs - which are concerns suddenly made separate from how the type hierarchy we want to use is implemented and the interface we want to use (copyable). It's also possible to generalize the idea to any type, and therefore, just promise that whatever is passed in such type-erasing type will be copied, and copyable if you copy the instance of that type-erasing type.

Something like that (assuming we want to only support a hierarchy of types inheriting from T) - (note: pseudocode):

template<class T>
class polymorphic_ptr
{
    // ...
public:
   using this_type  = polymorphic_ptr<T>;
 
   // ....

   // copy
   this_type& operator=(const this_type& other)
   {
       storage = other.storage->clone();
       return *this;
   }
   

private:
   struct interface
   {
       //...
       virtual interface* clone() = 0;
   };

   template< class U >
       requires std::is_base_of_v<T, U>
   struct model : interface
   {
       U value; // details depends on how the storage is implemented, could have been a pointer here
       // ...
       interface* clone() override 
       { 
          return new model{ value }; // or something similar, relying on the copy operations specific to U 
       } 
   };

   unique_ptr<interface> storage; // there are various ways to do this, I'll simplify here but dont assume it's always unique_ptr<interface>
   
};

Obviously this is a very incomplete example which is just made to provide a basic understanding of the technique used internally to generate the copy.

Examples of the techniques used in the generic version are often presented by Sean Parent in his talks, notably this one (slides) which is a more in-depth version of his talks about "Inheritance Is The Base Class Of Evil" (that I also recommend).

This paper about std::polymorphic and std::indirect describes exactly the rational for such type and interface it would have ideally and could be an interesting base to rely upon as a design fundation (note that this paper is not yet part of the standard, although it's in good progress, see for details ) (you might also want to check the paper that preceded this one)

This is not a request for this PR but just information for future design improvements :)

@JohanMabille JohanMabille merged commit 6c843d0 into man-group:main Sep 5, 2024
56 checks passed
@JohanMabille JohanMabille deleted the cloning_ptr branch September 5, 2024 15:58
@jbcoe
Copy link

jbcoe commented Sep 21, 2024

As the author of the indirect/polymorphic paper I’d be happy to discuss your design requirements in more detail and potentially provide you with a suitably licensed implementation of our proposed additions.

@JohanMabille
Copy link
Collaborator Author

@jbcoe thanks! We are currently swamped with other tasks, but happy to discuss when they're done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring: Implement a cloning smart pointer
4 participants