cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
16 stars 21 forks source link

Implementation: use std::unique_ptr rather than a raw point in our pImpl? #990

Closed agarny closed 2 years ago

agarny commented 2 years ago

Right now, we are using a raw pointer while it would be better to use std::unique_ptr (see https://herbsutter.com/gotw/_100/). The private implrementation shouldn't be shared, so to use std::unique_ptr makes sense. Also, it simplifies the code a bit. For instance, no more need for something like:

Component::~Component()
{
    delete pFunc();
}

Instead, we could simply have:

Component::~Component() = default;
hsorby commented 2 years ago

I did look at this a long time ago and found that compiler support was not consistent across OSes. From memory Visual Studio produced a warning which ruled out using std::unique_ptr. Maybe it is time to re-investigate if this is still the case.

agarny commented 2 years ago

I am about to try the std::unique_ptr approach in libOpenCOR which I test using windows-2019 in GitHub Actions, i.e. MSVC 2019. Will feedback here.

agarny commented 2 years ago

FWIW, std::unique_ptr works fine for me using MSVC 2019.

hsorby commented 2 years ago

And Linux and macOS?

agarny commented 2 years ago

Yes, all good using windows-2019, ubuntu-20.04, and macos-11 on GitHub Actions.

agarny commented 2 years ago

Ok, although it works fine for a "simple" pImpl approach, I am not sure it can for our inheritance-based pImpl approach. So, it looks like a raw pointer is still the way to go in our case.