2
\$\begingroup\$

I'm working on a personal project to build an open-source gui for git. I'm hoping to learn a lot from the project, and maybe produce something useful for folks as I do.

I'm making use of libgit2 to interface with git repositories. The library is implemented in C, and any time a new object is created (be it a commit, repository, branch, generic object) it's done by making a call into the library and passing a pointer to a pointer of where that object will be stored (foo**), and then these objects are deleted by the client code by making another libgit2 call to free the resource (e.g. git_repository_open and git_repository_free)

I want to get all the benefits of RAII types like std::unique_ptr but also make use of this library. Because we're using libgit2 function calls to create and destroy objects instead of the new, delete operators, I wrote my own adapter class to and make the calls to the right libgit2 functions but manage the created pointer in a std::unique_ptr:

https://github.com/StephenLHern/fons/blob/main/git/unique_libgit_ptr.hpp

#pragma once

#include <functional>
#include <git2.h>
#include <memory>

namespace fons::git
{
    template <typename T>
    class unique_libgit_ptr
    {
      public:
        template <class CreateMethodT, class... CreateMethodArgs>
        unique_libgit_ptr(CreateMethodT creator, std::function<void __cdecl(T *)> free_method, CreateMethodArgs &&...args)
        {
            T *rawPtr{};
            int createResult = creator(&rawPtr, std::forward<CreateMethodArgs>(args)...);

            if (createResult != GIT_OK)
                return;

            base_ptr = std::unique_ptr<T, std::function<void __cdecl(T *)>>{std::move(rawPtr), free_method};
        }

        T &operator*()
        {
            return &base_ptr;
        }

        T *operator->()
        {
            return *base_ptr;
        }

        operator bool()
        {
            return static_cast<bool>(base_ptr);
        }

        auto release()
        {
            return base_ptr.release();
        }

        auto reset(std::unique_ptr<T, std::function<void __cdecl(T *)>>::pointer ptr)
        {
            return base_ptr.reset(ptr);
        }

        auto get()
        {
            return base_ptr.get();
        }

        auto get_deleter()
        {
            return base_ptr.get_deleter();
        }

      private:
        std::unique_ptr<T, std::function<void __cdecl(T *)>> base_ptr;
    };

    using unique_commit_ptr = unique_libgit_ptr<git_commit>;
    using unique_revwalk_ptr = unique_libgit_ptr<git_revwalk>;
    using unique_git_obj_ptr = unique_libgit_ptr<git_object>;
    using unique_repo_ptr = unique_libgit_ptr<git_repository>;
    using unique_branch_itr = unique_libgit_ptr<git_branch_iterator>;
    using unique_ref_ptr = unique_libgit_ptr<git_reference>;

} // namespace fons::git

Here's how this class gets used, (unique_repo_ptr, unique_git_obj_ptr, unique_commit_ptr, etc.):

https://github.com/StephenLHern/fons/blob/main/git/commands/revwalk.cpp

#include "revwalk.hpp"
#include "app_main.hpp"

#include "git/git_observer.hpp"
#include "git/gitlib_manager.hpp"
#include "git/unique_libgit_ptr.hpp"

namespace fons::git
{
    wxDEFINE_EVENT(EVENT_REVWALK, revwalk_event);

    void revwalk::execute()
    {
        gitlib_manager gitlib;
        unique_repo_ptr repo(git_repository_open, git_repository_free, app->settings.active_repo.c_str());

        unique_revwalk_ptr walk(git_revwalk_new, git_revwalk_free, repo.get());

        if (!walk.get())
            return;

        git_revwalk_sorting(walk.get(), GIT_SORT_TOPOLOGICAL | GIT_SORT_TIME);
        git_revwalk_push_head(walk.get());
        git_revwalk_hide_glob(walk.get(), "tags/*");

        unique_git_obj_ptr obj(git_revparse_single, git_object_free, repo.get(), "HEAD~1000");
        git_revwalk_hide(walk.get(), git_object_id(obj.get()));

        git_oid oid;
        while (git_revwalk_next(&oid, walk.get()) == 0 && status != common::cmd_status::cancelled)
        {
            unique_commit_ptr commit(git_commit_lookup, git_commit_free, repo.get(), &oid);

            char oidstr[GIT_OID_HEXSZ + 1]{};
            std::string id(git_oid_tostr(oidstr, sizeof(oidstr), &oid));
            std::string message((git_commit_message(commit.get())));
            std::string author_email(git_commit_author(commit.get())->email);
            std::string author_name(git_commit_author(commit.get())->name);
            __time64_t timestamp = git_commit_time(commit.get());

            fons::git::commit commit_data(id, message, author_name, author_email, timestamp);
            queue_event<revwalk_event>(EVENT_REVWALK, EVENT_FOUND_COMMIT, commit_data);
        }

        return;
    }
} // namespace fons::git

I was interested in feedback on this code. Are there other ways to approach this problem I may not have considered? Or further ways to simplify the implementation? In particular I was wondering if there are ways to automatically deduce the template parameter for these pointers from the **foo arguments and eliminate the need for using alias statements.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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*).

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for taking the time to write out such detailed feedback, I'll definitely be incorporating this! I had hopped to avoid writing wrapper code for the libgit2 functions, but I think your recommendation will save me time as I start putting comprehensive error handling into the code, and run into other recurring issues that come with working with c-style libraries. The construction error-handling found its way into the pointer constructor as a way to avoid rewriting boilerplate code each time a libgit2 object is created. I think by templating wrapper code I can achieve something similar \$\endgroup\$ May 21 at 19:40

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.