std::unique_ptr adapter class template for libgit2 objects

I’m not sure this is a good way to go about solving this problem.

The first thing that bothers me is that in order for this strategy to work, you have to almost entirely reimplement std::unique_ptr… all just to get customized behaviour in the constructor. That seems excessive.

It also puts the onus on you to make that re-implementation as good as the original… which, it ain’t. Let’s just pick on a single function to illustrate; let’s use get():

// Your version's interface:
auto get() -> T*;

// std's interface:
auto get() const noexcept -> pointer;

Right away you can see that the standard version is superior, not only because it is guaranteed no-fail, but also because it can be used with a const instance.

Now you could go through and fix all these problems… but that would be missing the point. Trying to track the standard’s interface is a sucker’s game, because it can… has already… and probably will again in the future… change. Again, consider get(). The standard interface I gave above is valid for C++11—C++20… but as of C++23, it’s also going to be constexpr. Which means you’d have to use the preprocessor to check the standard version and conditionally add the constexpr.

But no, no, this is getting silly. So maybe trying to recreate std::unique_ptr’s interface isn’t the best plan.

The second thing that bothers me about this strategy is that will be very inefficient. The issue is in how you handle the deleter.

I actually don’t get why you’re using std::function at all in this case. It seems absurd. Just compare the size of a unique pointer with a std::function deleter, versus one with a bare function pointer, versus one with a deleter type. Using the bare function pointer—without std::function—makes it 2½× smaller. And there’s absolutely no benefit to using std::function here, because you always just use a plain C function as the deleter; never a lambda or bound function object or anything else that needs the extra storage std::function exists to manage.

But even the bare function pointer makes no sense, because you know, at compile time, what the deleter for any given type is. If T is a git_repository, then the deleter function is git_repository_free(). Always, always. Since that is statically known, there is no need for a function pointer to figure it out at runtime. Without the need for a function pointer, your unique pointer can be exactly the size of a bare pointer, with zero overhead when determining the deleter function.

So let me offer a first pass at an alternate strategy. Instead of trying to duplicate std::unique_ptr just to get a bespoke constructor, and instead of trying to muck about with function pointers to get custom deletion, how about this:

namespace fons::git {

struct deleter
{
    constexpr auto operator()(::git_commit* p) const noexcept { ::git_commit_free(p); }
    constexpr auto operator()(::git_revwalk* p) const noexcept { ::git_revwalk_free(p); }
    constexpr auto operator()(::git_object* p) const noexcept { ::git_object_free(p); }
    // ... and so on for every type and matching release function
};

} // namespace fons::git

// Usage example (bear with this for now):
::git_repository p = nullptr;

if (auto res = ::git_repository_open(&p, path); res != GIT_OK)
    throw some_error{};

auto repo = std::unique_ptr<::git_repository, fons::git::deleter>{p};

You could make it even nicer with a simple alias, and by taking advantage of template parameter deduction:

namespace fons::git {

struct deleter
{
    constexpr auto operator()(::git_commit* p) const noexcept { ::git_commit_free(p); }
    constexpr auto operator()(::git_revwalk* p) const noexcept { ::git_revwalk_free(p); }
    constexpr auto operator()(::git_object* p) const noexcept { ::git_object_free(p); }
    // ... and so on for every type and matching release function
};

template <typename T>
using unique_ptr = std::unique_ptr<T, deleter>;

} // namespace fons::git

// Usage example (bear with this for now):
::git_repository p = nullptr;

if (auto res = ::git_repository_open(&p, path); res != GIT_OK)
    throw some_error{};

auto repo = fons::git::unique_ptr{p};

But of course, that still leaves the ugliness of needing a temporary bare pointer, and the error handling.

The place to fix that, I would suggest, is not in the smart pointer, it is in a wrapper function around the problematic C function. For example:

namespace fons::git {

auto repository_open(char const* path) -> unique_ptr<::git_repository>;

// and, while we're at it, a handy overload so we don't need to do .c_str()
auto repository_open(std::filesystem::path const& path) -> unique_ptr<::git_repository>;

// and if you're storing paths as strings, i suppose you could use this, too
auto repository_open(std::string const& path) -> unique_ptr<::git_repository>;

} // namespace fons::git

// Usage example:
auto repo = fons::git::repository_open(path);

// and in case it isn't clear, repo is a
// std::unique_ptr<::git_repository, fons::git::deleter>
// which will be auto-released at the end of the scope.

The implementation of that wrapper function is trivial. In fact, we already wrote it:

auto repository_open(char const* path) -> unique_ptr<::git_repository>
{
    ::git_repository p = nullptr;

    if (auto res = ::git_repository_open(&p, path); res != GIT_OK)
        throw some_error{};

    return fons::git::unique_ptr{p};
}

What about the some_error? I’d recommend looking into std::error_code, making an error category for libgit2, and setting up the system_error machinery, so instead of throw some_error{}, you’d do throw std::system_error{res, fons::git::error_category()} (which, of course, could be simpler if you made an enum for git errors, yadda yadda… down to throw std::system_error{fons::git::errc(res)}; see a system_error tutorial for details). Then you could do like the filesystem library does, and have throwing and non-throwing functions:

namespace fons::git {

// will throw a std::system_error if opening the repo fails.
auto repository_open(char const* path) -> unique_ptr<::git_repository>;

// will *not* throw if opening the repo fails, but you can query ec
auto repository_open(char const* path, error_code& ec) -> unique_ptr<::git_repository>;

} // namespace fons::git

// Usage:

//  In this case, I'm *NOT* expecting the open to fail; if it does, that would be weird
//      auto repo = fons::git::repository_open(path);

//  In this case, I *EXPECT* the open to fail, sometimes
//      auto ec = std::error_code{};
//      auto repo = fons::git::repository_open(path, ec);
//      if (not ec)
//          // open was okay
//      else
//          // handle error; ec contains details about error

In practice, I’d use the simpler version when I wasn’t really expecting problems, and if a repo fails to open, I could just catch the exception at a higher level and report via message box or something. I’d use the error_code version when a failure isn’t entirely unlikely, and I have a strategy for what to do if one happens (like, maybe try an alternate path, or try a recovery, or whatever). (Or if you really want to get high-tech, don’t make the same mistake the standard filesystem library did, with its double interface, and maybe look into expected, or outcome, or LEAF, or whatever else instead.)

But in all this the bottom line is: there is no need to roll your own smart pointer. All the standard pointers will work just fine. Just pass your custom deleter—and use a type, not function pointers—and you can use unique_ptr, shared_ptr, whatever.

The place to handle all that error-handling logic isn’t in a pointer constructor, it’s in wrappers around the clunky C functions. Which are a good idea anyway, because you can fix all the other bad C interface crap (like using strings instead of char const*).