acts-project / vecmem

Vectorised data model base and helper classes.
https://acts-project.github.io/vecmem/
Mozilla Public License 2.0
19 stars 13 forks source link

Memory Resource Re-Design, main branch (2023.10.12.) #240

Closed krasznaa closed 1 year ago

krasznaa commented 1 year ago

In a bit of a monstrous PR, I re-designed vecmem::details::memory_resource_base a little. It is now a project specific interface, which mimics std::pmr::memory_resource, but doesn't actually inherit from it.

In addition I created vecmem::details::memory_resource_adaptor, which could be used to provide classes implementing the std::pmr::memory_resource interface using classes that implement the vecmem::details::memory_resource_base interface.

I also split most of the memory resources in the vecmem::core library in two. A "main class" implementing the vecmem::details::memory_resource_base interface, and an "implementation class" that doesn't inherit from anything, it just implements the behaviour of the class in the appropriate way, with a PIMPL design.

The goal of all of this was to get rid of a whole lot of MSVC warning suppressions from the code. For this purpose I couldn't even use std::unique_ptr in my PIMPL implementation, as that would've completely undone the whole purpose of why I'm separating the implementation from the interface...

While at it, I also made some changes to unify the implementation of the different memory resources a bit. Since at this point, why not... 😛

The public API of the project does not change with this update. In the end I did need to update one of the unit tests, but just because that test was using vecmem::details::memory_resource_base directly. The ABI of the libraries does change significantly however. So if you guys are okay with this change, I think this will be the point at which we could tag version 1.0.0, with a new library ABI version. 😉 (Switching it from 0 to 1.)

krasznaa commented 1 year ago

@stephenswat, if you have any objections, let me know shortly. As I plan to push this in, in not too long.

stephenswat commented 1 year ago

Sure, go ahead.