Skip to content

Conversation

@yungyuc
Copy link
Member

@yungyuc yungyuc commented Nov 20, 2020

This PR suggests to allow the internal data buffer of ConcreteBuffer to be owned outside the shared-pointer-controlled object. Before the change, creation of ConcreteBuffer mandates dynamic memory allocation.

A custom std::unique_ptr Deleter is added to support memory management of the internal buffer of ConcreteBuffer. The customer Deleter uses a polymorphic "remover" to control the lifecycle of the internal buffer. By default, the remover is null, and the Deleter behaves the same as std::default_delete<T>.

The remover makes it possible to bind the lifecycle of the internal data buffer outside. For example, struct ConcreteBufferNdarrayRemover is derived from ConcreteBuffer::remover_type and bind the lifecycle of the buffer to a PyArrayObject (i.e., pybind11::array).

@yungyuc yungyuc added the enhancement New feature or request label Nov 20, 2020
@yungyuc yungyuc self-assigned this Nov 20, 2020
@yungyuc yungyuc marked this pull request as draft November 20, 2020 14:47
* Take remover and deleter classes outside ConcreteBuffer to work around for a bug in g++ 7: https://bugzilla.redhat.com/show_bug.cgi?id=1569374 .
* Fix gcc visibility in the pybind11 wrapper file.
* Add default constructor of ConcreteBufferNdarrayRemover to make it work with unique_ptr
* Supply the deleter object to unique_ptr constructor.
* Remove unnecessary else.
@yungyuc
Copy link
Member Author

yungyuc commented Nov 20, 2020

@tigercosmos This may be an example for the 'system-level' work that should be done for the mesh code base. For your information. Feel free to comment.

@yungyuc yungyuc marked this pull request as ready for review November 22, 2020 02:37
@yungyuc yungyuc merged commit 749409f into master Nov 22, 2020
@yungyuc yungyuc deleted the feature/cb-owner branch November 22, 2020 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants