shared_ptr.unique() prevents C++20 / Native windows build

[LeeReid1]

Hi guys,

I need to preface this by highlighting C++ is not my mainstay and I've not your depth of knowledge on this codebase, so am a little out of my depth here.

I've locally branched the dev arm to put together a native windows build (i.e. MSVC). MSVC cannot resolve some of the more complex SFINAE code in 2-3 files, and bugs-out on a supporting library, but this is resolvable by switching to C++20 concepts. Suffice to say it's an absolute blackhole time-wise to find a work-around, if staying in C++17. My WIP currently should still compile as normal on linux (C++17) and is almost there on Windows with C++20 for most executables.

However, the compilation fails due to shared_ptr.unique() in image.h. This method is deprecated in C++17 and eliminated in C++20 as it's not threadsafe and checks are likely avoidable by being careful with code architecture.

I can't help but wonder about the code using it, but my knowledge of how mrtrix is put together is not deep enough to fix this without potentially causing an issue elsewhere. Hence, I'm seeking your opinions on solutions.

The primary use is in the destructor of image.h, where it seems to be used to check if this is the only user of the buffer. What follows seems to be unbuffering back to some original location(?):

template <typename ValueType> Image<ValueType>::~Image() {
  if (buffer.unique()) { 
    // was image preloaded and read/write? If so,need to write back:
    if (buffer->get_io()) {
      if (buffer->get_io()->is_image_readwrite() && buffer->data_buffer) {
        auto data_buffer = std::move(buffer->data_buffer);
        TmpImage<ValueType> src = {
            *buffer, data_buffer.get(), std::vector<ssize_t>(ndim(), 0), strides, Stride::offset(*this)};
        Image<ValueType> dest(buffer);
        threaded_copy_with_progress_message("writing back direct IO buffer for \"" + name() + "\"", src, dest);
      }
    }
  }
}

I find it odd that an image may not be the owner or sole consumer of its own buffer in any read/write scenario. Other code suggests that, actually, this situation should never occur:

template <typename ValueType> Image<ValueType> Image<ValueType>::with_direct_io(Stride::List with_strides) {
  if (buffer->data_buffer)
    throw Exception("FIXME: don't invoke 'with_direct_io()' on images already using direct IO!");
  if (!buffer->get_io())
    throw Exception("FIXME: don't invoke 'with_direct_io()' on non-validated images!");
  if (!buffer.unique())
    throw Exception("FIXME: don't invoke 'with_direct_io()' on images if other copies exist!");

That suggests to me that the buffer.unique() check in ~ could simply be removed, and perhaps this safety check in with_direct_io could be avoided by use/check-for a unique_ptr or similar. If that's enough, I'm happy.

I believe there's another workaround I've not attempted, which would be something like so:

bool owns_buffer(){
  std::weak_ptr<int> wp = buffer;
  if (auto locked = wp.lock()) {
    return true;
  }
  return false;
}

That said, I can't help but wonder if there's a deeper issue. OOP principles suggest an object should only create/expose its data in a 'safe' way, eliminating the need for such checks. So, if Image1 opens a file and needs to make a buffer, noone else should be able to take control of that buffer, and if it's exposed it's up to anyone reading it to stop doing so before the owner is destroyed. If the dependency injection pattern is relied upon, the creator of resources would be responsible for their clean-up/finalisation, not the consumer of them. So, if Image1 is provided a buffer to read/write to, it is not responsible for what happens to that buffer after it's finished its work.

For example:

• Image opens file x.mif
• Image creates a buffer, or some translation layer, to be able to read/write to it
• Image cleans up resources when done

Or with DI:

• Image1 accepts a dataProvider agnostically
• Image1 reads/writes to dataProvider as needed
• dataProvider takes responsibility to convert that into appropriate I/O operations (to disk/memory/etc).
• Either the dataProvider handles clean up in its destructor, or the creator of Image1 explicitly cleans it up when Image1 is finished being used.

The current code looks like it's trying to use DI, but then on its destructor take responsibility for figuring out if it's been abandoned and needs to be a good citizen and complete some IO operation that presumably was intended by its creator(?).

I've probably misinterpreted something due to lack of context but any help you could offer eliminating these unique() calls would be really appreciated. Will offer the native build back as a PR if I can get it over the line.

Thanks

[daljit46]

Hi, you can try to replace unique() with use_count() == 1 (see here). This should at least make that compile. Whether those checks can be removed needs more thinking, but I agree that we should try to get rid of them. Getting a native MSVC build would be nice and something I'd like to make viable just to use all the Visual Studio tools, but I haven't attempted that yet as currently the codebase is quite Unix-centric.

[LeeReid1]

Can confirm that works! Many thanks. Given my goal is just to get it working I won't attempt any kind of reworking of the architecture leading to the problem. Up to you whether you'd like to close this issue or leave it open as something to review at a later stage.