Code review comment for lp:~pete-woods/unity-api/add-glib-assigner-class

Revision history for this message
James Henstridge (jamesh) wrote :

Looking at the TypeName##Assigner class in GlibMemory.h, do we really need the UPtr here? It looks like all we really want is the deleter function, but that is tied up in the implementation of unique_glib(), since that's where the function is actually used in the expanded code.

Maybe the code could be simplified a bit by leaning in to the template code a bit more. If we instead define GlibDeleter something like this:

    template<typename T> struct GlibDeleter;

#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
    template<> struct GlibDeleter<TypeName> { \
        void operator()(TypeName* ptr) { \
            if (ptr != nullptr) { \
                func(ptr); \
            } \
        } \
     };

Now in the assigner class destructor, you could delete the pointer directly with GlibDeleter<TypeName>()(_ptr).

You could also define unique_glib/shared_glib outside of the macro expansion with something like:

template<typename T>
inline std::unique_ptr<T,GlibDeleter<T>> unique_glib(T* ptr
{
    return std::unique_ptr<T, GlibDeleter<T>>(ptr);
}

... which will only match types for which GlibDeleter<T> has a definition.

I suspect you could also use this to define a single templated Assigner class too.

« Back to merge proposal