4
\$\begingroup\$

I have a situation where I'd like to wrap up some plain C handles into a class, with a destructor and other niceties, in C++. I would like the wrapper class to have the exact same size as the handle, so I can pass an array of the wrappers into the C api without having to allocate and translate them first.

The handle is not unique - it has some extra information, which is known at creation time - so I can't use the handle itself as a key in one big map. Instead, I'm trying to use the address of the handle as the key.

Details aren't really important on this one - it has data, savvy?

class InstanceMetadata { ... };

The header for the wrapper class:

class Instance
{
public:
    Instance(const Instance&) = delete;
    Instance(Instance&& other);
    Instance(InstanceHandle handle, InstanceMetadata metadata);
    ~Instance();

    Instance& operator=(Instance&) = delete;
    Instance& operator=(Instance&& other);

    InstanceHandle GetHandle() const;
    void ResetHandle();

private:
    InstanceHandle handle;
};

A thin wrapper around std::map, which handles synchronization and nullptrs

template<typename Handle, typename Metadata>
class MetadataCache
{
public:
    void Set(Handle& key, Metadata* value)
    {
        std::lock_guard<std::mutex> guard(mutex);
        data[&key] = value;
    }

    Metadata* Get(Handle& key)
    {
        std::lock_guard<std::mutex> guard(mutex);
        std::map<Handle*, Metadata*>::iterator it = data.find(&key);
        if (it == data.end())
            return nullptr;
        return it->second;
    }

    void Erase(Handle& key)
    {
        std::lock_guard<std::mutex> guard(mutex);
        std::map<Handle*, Metadata*>::iterator it = data.find(&key);
        if (it != data.end())
            data.erase(it);
    }

private:
    std::mutex mutex;
    std::map<Handle*, Metadata*> data;
};

The implementation for the wrapper class:

using namespace std;
using namespace vulkan;

MetadataCache<InstanceHandle, InstanceMetadata> cache;

Instance::Instance(Instance&& other) :
    handle(other.handle)
{
    other.handle = InstanceHandle();
    cache.Set(this->handle, cache.Get(other.handle));
    cache.Erase(other.handle);
}

Instance::Instance(InstanceHandle handle, InstanceMetadata metadata) :
    handle(handle)
{
    cache.Set(handle, new InstanceMetadata(metadata));
}

Instance::~Instance()
{
    delete cache.Get(handle);
    if (handle)
        vkDestroyInstance(handle, nullptr);
}

Instance& Instance::operator=(Instance&& other)
{
    delete cache.Get(handle);
    cache.Set(handle, cache.Get(other.handle));
    cache.Erase(other.handle);
    other.handle = InstanceHandle();
    return *this;
}

InstanceHandle Instance::GetHandle() const
{
    return handle;
}

void Instance::ResetHandle()
{
    delete cache.Get(handle);
    handle = InstanceHandle();
}

The default constructor for InstanceHandle creates a null handle.

I'm still fairly new to move-semantics, so the main things I would like to know are:

  • Is it safe to do this? (i.e. have I missed an operator or something?)
  • Is there another way to do this?
  • Is there any good reason not to do this?

Keep in mind I don't really care about performance, as these objects will not be created or destroyed very often.

\$\endgroup\$
  • 1
    \$\begingroup\$ Don't fancy using auto for those long map::iterator declarations? ;) \$\endgroup\$ – glampert Apr 23 '16 at 2:43
  • \$\begingroup\$ No particular reason. Maybe I'm old school... \$\endgroup\$ – Andrew Williamson Apr 23 '16 at 2:50
  • \$\begingroup\$ Hehe, okay. Then I'd suggest typedefing them at least, to make life easier if you say, decide to switch to an unordered_map tomorrow. \$\endgroup\$ – glampert Apr 23 '16 at 2:58
2
\$\begingroup\$

Because I also had to deal with a lot of handles, used by for example the Win32 API, I really can understand your use case.

Is it safe to do this? (i.e. have I missed an operator or something?)

I don't think so, for a lot of reasons: Let's assume the handle your library defines is an opaque type, usually a void*. So the first problem is that the C-library has no idea of C++ concepts like operator overloading. So it will not call your operator= i.e. nor will the compiler disallow to assign named instances. Furthermore the library will deference your handle, assuming it points to it's internal data structure of some kind. Also the compiler has no idea that you have given the library a instance different from handle, while translating the library code. So even compiling the library with a C++ compiler won't make a difference.

Is there another way to do this?

Although you're desire to design a nice C++ wrapper is a good idea, I would drop the idea to slip the C-library your type. I recommend to just define a .native_handle() member function or overload the operator* but then I would suggest that the name of your type does stress the wrapper nature more.

Is there any good reason not to do this?

Despite from the reasons above, the size would already change if someone would sub or base class it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer - you make a good point about the C-library. My thoughts were that it didn't matter if the C library doesn't use operator=, because it is not going to call a destructor either. How would you recommend I handle the cases where I have an array of wrappers, and have to pass in an array of handles? I'd like this to be efficient, because it happens very frequently. \$\endgroup\$ – Andrew Williamson Apr 25 '16 at 20:54
  • \$\begingroup\$ You could write a nice container or bundle class for your type (if I understand you correctly you were about to anyway). So let's assume your container class, uses a standard container as a private(!) member to manage your wrapper_handle_types, like std::vector or std::map, it doesn't matter. From there I would implement a method that returns a const std::vector<c_handle_types>. And from this vector you can get the array by std::vector::data(). Your wrapper class also could cache such an vector for performance, but I think my solution is fast enough. \$\endgroup\$ – Superlokkus Apr 25 '16 at 21:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.