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

Enable shared_ptr to work without ContainerTraits::construct #86

Merged
merged 3 commits into from
Apr 3, 2023

Conversation

kunitoki
Copy link
Owner

@kunitoki kunitoki commented Apr 1, 2023

No description provided.

@kunitoki kunitoki changed the title Enable shared_ptr to work without get Enable shared_ptr to work without ContainerTraits::construct Apr 1, 2023
@rpatters1
Copy link
Contributor

This addresses my issue. I'm having some other strange migration issues, which i will report separately, but this addresses the need for RefCountedPtr in my project.

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 2, 2023

You can do:

template <class T>
using RefCountedPtr = std::shared_ptr<T>;

To minimize the breaking change

@rpatters1
Copy link
Contributor

Yes, I did that to test it. It was a little more involved than that, because the raw pointer constructor for std::shared_ptr is explicit where that for RefCountedPtr is not. But basically that's what I did.

Thank you for taking time to address this.

@rpatters1
Copy link
Contributor

Also, another question. There is one of my classes that is not part of the legacy framework. I am going to "intrude" on it to make it behave better. Do you recommend RefCountedObject or std::enable_shared_from_this? Are there any advantages to the one over the other?

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 2, 2023

I think enable_shared_from_this is the standard way to handle intrusive shared ownership in C++ and it's backed by the standard libraries. I will eventually deprecate RefCountedObject in the future.

@rpatters1
Copy link
Contributor

If cross-dll boundary support is a strong goal, there should perhaps be a built-in way to create shared pointers that use the destructor function from the DLL that created them, something like this:

template<class T>
static std::shared_ptr<T> makeLuaPtr(T *p) { return std::shared_ptr<T>(p, [](T* p){ delete p; }); }

@rpatters1
Copy link
Contributor

rpatters1 commented Apr 2, 2023

If you are going to deprecate RefCountedObject, is the following

class foobase : public std::enable_shared_from_this<foobase> {};
class foo : public foobase {};

going to work equivalently to

class foobase : public luabridge::RefCountedObject {};
class foo : public foobase {};

That is, does type-based instantiantion std::enable_shared_from_this<foobase> still allow subclass foo to work as expected? Otherwise, I would recommend against deprecating RefCountedObject unless it is actually broken.

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 2, 2023

Yes it does, plus you gain weak references that are not possible with RefCountedObject. And you have documentation and standard support https://en.cppreference.com/w/cpp/memory/enable_shared_from_this

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 2, 2023

If cross-dll boundary support is a strong goal, there should perhaps be a built-in way to create shared pointers that use the destructor function from the DLL that created them, something like this:

template<class T>
static std::shared_ptr<T> makeLuaPtr(T *p) { return std::shared_ptr<T>(p, [](T* p){ delete p; }); }

Isn't this easy to roll your own ?

I'm a bit against providing these constructs, as they make no interaction at all with the library itself, they are more belonging to C++ than luabridge. Where is *p allocated ? Who allocates it ? What if by mistake someone see the method and allocates a T* from the main binary and then it's deallocated it in the dynamic library ? What if i have a specific destructor ? Should i make a makeLuaPtr for custom constructors and destructors as well ?

Those constructs are really flexible if solved in the implementation layer, and the library should just make sure they work with lower level primitives.

@rpatters1
Copy link
Contributor

rpatters1 commented Apr 2, 2023

Given that I can put std::enable_shared_from_this on my base class that everything inherits, I am leaning heavily towards using it everywhere, at least when compiling with LB3. I would probably go ahead and use RefSharedObject with LB2 if it would compile under Objective C++. (I'm glad to see LB3 addresses this.)

In the meantime, it's not so much a question of how easy something is but whether the programmer knows to do it. If a stated feature of the library is cross-DLL support, it would be nice if programmers didn't have to know these things in order to have it. But I don't feel strongly about it.

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 2, 2023

I've recently started trying out sonar, and RefCountedObjectand RefCountedObjectPtr are hives of bugs and code smells. Now that we have support for a standard construct like shared_ptr and enable_shared_from_this, it might be time to kill those old relics.

On the dll support, it's a bit more complicated than just a makeLuaPtr helper. A proper plugin system must define factory constructors and destructors methods, that's a safe way to deal with dll boundaries of allocations. Then for shared libraries to register via luabridge (https://github.com/kunitoki/LuaBridge3/blob/master/Tests/Source/SharedCode.cpp) for example, lua needs to be configured so all internal allocations don't happen in the dll (https://github.com/kunitoki/LuaBridge3/blob/master/Tests/Source/DynamicLibraryTests.cpp#L161-L214).

@rpatters1
Copy link
Contributor

rpatters1 commented Apr 2, 2023

I compile Lua directly into my plugin, fwiw. I'm not super concerned about cross-dll support in my environment, tbh. My biggest concern is that I am a single point of failure for the project. I am the only person on Earth who has access to and permission to use the proprietary code needed for this project. (Well, there are 2 others. One is the original creator of the legacy framework, but he has withdrawn from the internet, and the other is not interested.)

Cross-dll support might allow for external user interfaces. Right now my bandwidth is limited, and I am not terribly interested in adding new ui support.

EDIT: I should clarify I am the only 3rd party person with access and permission. Obviously the employees of the company do.

@kunitoki kunitoki merged commit 4b7dff5 into master Apr 3, 2023
@kunitoki kunitoki deleted the dev/no_enable_shared_from_this branch April 3, 2023 20:50
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.

2 participants