Your understanding is correct: if the deleter has a member type pointer, then std::unique_ptr uses that to define its own member type pointer, and from that, most other operations. So long as you don’t do anything too complicated, it should “work”. Since you only seem to be doing sets and gets… then, yeah, it shouldn’t be a problem.

But is it a good idea? I would say no.

Try this:

using unique_float_ptr = std::unique_ptr<float>;
using unique_magic_ptr = std::unique_ptr<magic_t, MagicDeleter>;

static_assert(
    not std::same_as<
        unique_float_ptr::element_type,
        unique_float_ptr::pointer
    >
);
static_assert(
    not std::same_as<
        unique_magic_ptr::element_type,
        unique_magic_ptr::pointer
    >
);

As you can see, you’ve kinda warped the abstraction. element_type and pointer should not be the same in a unique_ptr; that just breaks the model.

If magic_t is an opaque handle type, and you don’t want to crack it apart to its constituent types, you can still do this:

struct MagicDeleter
{
    using pointer = ::magic_t;

    void operator()(pointer cookie) noexcept
    {
        if (cookie)
            ::magic_close(cookie);
    }
};

using unique_magic_ptr = std::unique_ptr<std::remove_pointer_t<::magic_t>, MagicDeleter>;

If magic_t really is a pointer, then the unique pointer will be “normal”. If it is not, then it will still “work”, but will have the weird behaviour that element_type and pointer are the same… but that can’t be helped if magic_t really is opaque. In that case, it probably shouldn’t be in a unique_ptr… because it’s not a pointer.

If I were going to use something like libmagic in a C++ program, I would wrap the C interface in a proper C++ interface, like so:

namespace magic {

struct deleter
{
    auto operator()(::magic_t p) const noexcept
    {
        if (p)
            ::magic_close(p);
    }

    // Any other types in the library that need a release function...
    auto operator()(::magic_other_t p) const noexcept
    {
        // ...
    }
};

// Good for a start, if `::magic_t` really is a pointer. If not, then
// a custom wrapper should be used.
//
// A custom wrapper should probably be used in the long run in any case
// for even more safety and power.
//
// For example, in the long run, instead of:
//  ::magic_load(m_cookie.get(), nullptr)
// or even: 
//  load(m_cookie)
// we might prefer:
//  m_cookie.load()
using magic_t = std::unique_ptr<std::remove_pointer_t<::magic_t>, deleter>;

enum class open_flags : int
{
    none,
    mime_type,
    // ... etc. ...
};

auto open(int flags) -> magic_t; // legacy function
auto open(open_flags) -> magic_t;

// Not necessary, but helpful when porting legacy code:
auto close(magic_t& m)
{
    m.reset();
}

auto load(magic_t& m) -> void;
auto load(magic_t& m, std::filesystem::path const&) -> void;
// legacy functions:
auto load(magic_t& m, std::nullptr_t) -> int;
auto load(magic_t& m, char const*) -> int;

} // namespace magic

Then you could start with C-ish code like:

auto m = ::magic_open(MAGIC_MIME_TYPE);
if (not m)
    throw error{};

if (::magic_load(m, nullptr) == -1)
{
    ::magic_close(m);
    throw error{};
}

::magic_close(m);

… and gradually change it like so:

auto m = magic::open(MAGIC_MIME_TYPE);
if (not m)          // this is now unnecessary, because open throws
    throw error{};  // on failure, but it's harmless

if (magic::load(m, nullptr) == -1)  // the if, the check, and the
{                                   // error block are now unnecessary
    magic::close(m);                // but harmless for now
    throw error{};
}

magic::close(m); // unnecessary:

… and then eventually start removing the unnecessary code to get:

auto m = open(magic::open_flags::mime_type);

load(m);

… where everything is strongly-typed, and “smart”, and errors become impossible to miss or ignore.

By making the new, better C++ interface mostly track the original C interface, you can make adoption gradual. And one of the keys to doing that is to make sure that abstractions line up. In particular, the “new” magic_t should not be a (unique) pointer to a magic_t, it should be a magic_t (even if it happens to be a (unique) pointer to something (to whatever the original magic_t points to) as an implementation detail).

posted 12 days ago

CC BY-SA 4.0

Indi‭

142 reputation 1 7 14 3