Merge into trunk : add-glib-assigner-class : Code : Unity API

Revision history for this message

Charles Kerr (charlesk) wrote on 2017-03-20:

#

LGTM.

review: Approve

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-20:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/155/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-21:

#

I'm in two minds about this.

auto o = unique_glib(g_foobar(args..., GErrorAssigner(error)));

I don't like the need to explicitly use the GErrorAssigner here. With an implicit conversion to GError**, this would not be necessary.

I'm also not that keen on the macro hackery. I know, it's not your fault… :-)

How about having a class instead that is specific to GError and provides the implicit conversion?
That way, I can write

AutoGerror error;
auto o = g_foobar(args..., error);

Call the class AutoGError maybe? Or maybe GErrorGuard would be a better name?

Something along these lines:

http://pastebin.ubuntu.com/24219982

I think this would be easier to use and also easier to understand in terms of how it is implemented?

review: Needs Information

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-21:

#

And, oops, the test in operator GEError**() is redundant. It should be:

operator GError** noexcept
{
g_clear_error(&err_);
return &err_;
}

because g_clear_error() does check whether err_ or *err_ is NULL.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-21:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/156/rebuild

review: Approve (continuous-integration)

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-21:

#

So one of the design goals I had for this was to use the built in unique_ptr and shared_ptr classes as much as possible:
* Developers are familiar with their semantics.
* unique_ptr "moves" to shared_ptr implicitly.

I agree it'd be nicer to try and make this a template, so I'll see if I can get that to work.

I plan to (if I can get it to work) make the technique also apply to glib style property getters (mainly for char arrays).

e.g. instead of:

gchar* a = nullptr;
gchar* b = nullptr;
g_get_properties(o, "a", &a, "b", &b, nullptr);
g_free(a);
g_free(b);

doing:

GCharUPtr a, b;
g_get_properties(o, "a", Assigner<gchar>(a), "b", Assigner<gchar>(b), nullptr);

I would also like it to apply generally to any glib or gobject type that is allocated this way.

e.g. a factory method:

unique_ptr<GApple> cpp_get_apple() {
    unique_ptr<GApple> a;
    if (!g_get_apple(_gFactory.get(), Assigner(a))) {
        throw some_error();
    }
    return a;
}

which I can then assign to a shared_ptr:

shared_ptr<GApple> a(cpp_get_apple());
other_method_call_that_wants_a_shared_ptr(a);

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-21:

#

I also don't like the surprising automatic destruction of the wrapped object if someone decides to convert it to a double pointer. It makes me think of auto_ptr.

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-21:

#

It's difficult to get over all the friction here.

My gut feeling is that using the existing glib unique_ptr is a bit awkward. Not because there is anything wrong with the unique_ptr per se, but because the usage pattern here is different:

GError is conceptually a value type. The only reason it is dynamically allocated is because it is an out param that can be set to null to indicate "no error". So, the unique_ptr idea works well for return types, but I don't think it works all that well for out params.

I'd live with the unique_ptr thingy, if we could come up with a way to get rid of the Assigner. It really would be nice not to have to do that.

The destruction of the wrapped object isn't surprising, in my opinion. It simply deallocates any old value before possibly accepting a new value (which may be null). There has to be a way to easily re-used the same GError for several calls in a row, I think (whether a previous call succeeded or not). In effect, every time I pass a GError to a glib call, I know that I'll get a new value back, some error or a no error, with no possibility of leaking memory.

Do we need to support the T** conversion for potentially any and all glib pointer types, or just for some of them? I'm thinking that it might be possible to have a derived template that adds the conversion operator and then specialise that template for different glib types?

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-21:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/157/rebuild

review: Approve (continuous-integration)

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-21:

#

I've put together the sort of thing I was talking about now. This includes the same sort of thing for both char array stuff and GObject instances.

If you can figure out a way to do the Glib side with derived templates I'm all ears. I struggled very muchly with the template instantiation order learning some new things on the way when I just tried to make the GlibAssigner a template instead of macro funkiness.

In terms of the deletion being surprising, let's give an example, assuming we've got a wrapper type that auto converts to **:

void do_stuff(Thingy** t) {}

auto t = unique_glib(new_thingy());
do_stuff(t);
// t is now empty!

I don't like that, and it really does feel like the same problem as the old auto_ptrs:

void do_stuff(auto_ptr<Thingy> t) {}

auto_ptr<Thingy> t(new Thingy());
do_stuff(t);
// t is now empty!

I also don't think you're putting across a concrete argument about why the "Assigners" are bad. Is it just because there's more typing? Can you express more clearly what the problem is, please.

I prefer them because the "wacky types that do magic stuff" only live very briefly for the duration of a function call. Instead of having smart pointers with unusual properties that live potentially a long time.

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-21:

#

> I've put together the sort of thing I was talking about now. This includes the
> same sort of thing for both char array stuff and GObject instances.
>
> If you can figure out a way to do the Glib side with derived templates I'm all
> ears. I struggled very muchly with the template instantiation order learning
> some new things on the way when I just tried to make the GlibAssigner a
> template instead of macro funkiness.

I'll have a look tomorrow and see whether I can come up with something.

> In terms of the deletion being surprising, let's give an example, assuming
> we've got a wrapper type that auto converts to **:
>
> void do_stuff(Thingy** t) {}
>
> auto t = unique_glib(new_thingy());
> do_stuff(t);
> // t is now empty!
>
> I don't like that, and it really does feel like the same problem as the old
> auto_ptrs:
>
> void do_stuff(auto_ptr<Thingy> t) {}
>
> auto_ptr<Thingy> t(new Thingy());
> do_stuff(t);
> // t is now empty!

Not sure there is anything surprising here at all. It's exactly the same as passing any value by non-const reference. When the call returns, the value may or may not have changed. I expect that: after all, I'm passing the value so it *can* be changed by the called function.

> I also don't think you're putting across a concrete argument about why the
> "Assigners" are bad. Is it just because there's more typing? Can you express
> more clearly what the problem is, please.

It feels a bit clunky to have to explicitly instantiate an assigner instance just so I can pass the error instance. I have to do this each and every time, and there is no other use case for the Assigner class, as far as I can see. The assigners aren't bad, but the usage looks a bit inelegant to me. Ideally, I'd like to have a type that is a straight drop-in replacement for GError and can be passed wherever GError** is expected by a glib function, without any extra noise.

> I prefer them because the "wacky types that do magic stuff" only live very
> briefly for the duration of a function call. Instead of having smart pointers
> with unusual properties that live potentially a long time.

The way I wrote AutoGError, I don't think there is any magic stuff. It behaves like a value type. The only thing I can do is ask it whether the value exists (operator bool()), and access the value (via operator-> and operator*). If there is no value, and I try to use it, I get undefined behavior, just as I do with unique_ptr. If I pass it "by reference" where a glib function expects a GError "by reference", I'm not going to be surprised if an old error is replaced with a newer one if I reuse the same error instance.

In essence, this boils down to the question "should out params be syntactically distinguishable at the point of call?" In C++, the answer is usually "no" because, when a parameter can be changed by a called function, it normally is passed by non-const reference. In C#, you have to explicitly decorate the parameter at the point of call instead. (I do have sympathy for both approaches.)

> I've put together the sort of thing I was talking about now. This includes the
> same sort of thing for both char array stuff and GObject instances.
> 
> If you can figure out a way to do the Glib side with derived templates I'm all
> ears. I struggled very muchly with the template instantiation order learning
> some new things on the way when I just tried to make the GlibAssigner a
> template instead of macro funkiness.

I'll have a look tomorrow and see whether I can come up with something.
 
> In terms of the deletion being surprising, let's give an example, assuming
> we've got a wrapper type that auto converts to **:
> 
> void do_stuff(Thingy** t) {}
> 
> auto t = unique_glib(new_thingy());
> do_stuff(t);
> // t is now empty!
> 
> I don't like that, and it really does feel like the same problem as the old
> auto_ptrs:
> 
> void do_stuff(auto_ptr<Thingy> t) {}
> 
> auto_ptr<Thingy> t(new Thingy());
> do_stuff(t);
> // t is now empty!

Not sure there is anything surprising here at all. It's exactly the same as passing any value by non-const reference. When the call returns, the value may or may not have changed. I expect that: after all, I'm passing the value so it *can* be changed by the called function.

> I also don't think you're putting across a concrete argument about why the
> "Assigners" are bad. Is it just because there's more typing? Can you express
> more clearly what the problem is, please.

It feels a bit clunky to have to explicitly instantiate an assigner instance just so I can pass the error instance. I have to do this each and every time, and there is no other use case for the Assigner class, as far as I can see. The assigners aren't bad, but the usage looks a bit inelegant to me. Ideally, I'd like to have a type that is a straight drop-in replacement for GError and can be passed wherever GError** is expected by a glib function, without any extra noise.

> I prefer them because the "wacky types that do magic stuff" only live very
> briefly for the duration of a function call. Instead of having smart pointers
> with unusual properties that live potentially a long time.

The way I wrote AutoGError, I don't think there is any magic stuff. It behaves like a value type. The only thing I can do is ask it whether the value exists (operator bool()), and access the value (via operator-> and operator*). If there is no value, and I try to use it, I get undefined behavior, just as I do with unique_ptr. If I pass it "by reference" where a glib function expects a GError "by reference", I'm not going to be surprised if an old error is replaced with a newer one if I reuse the same error instance.

In essence, this boils down to the question "should out params be syntactically distinguishable at the point of call?" In C++, the answer is usually "no" because, when a parameter can be changed by a called function, it normally is passed by non-const reference. In C#, you have to explicitly decorate the parameter at the point of call instead. (I do have sympathy for both approaches.)

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-21:

#

My point about the surprising thing is that even if the method you hand it over to does absolutely nothing, then the pointer is still freed.

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-21:

#

> My point about the surprising thing is that even if the method you hand it
> over to does absolutely nothing, then the pointer is still freed.

True. But the only time anything ever will call operator**() is when they make a glib function call. In that case, the error is either null already or it contains a pre-existing error. In the latter case, it *must* be released first (and the caller would have to do the same thing manually otherwise). Either way, there is no surprise here: a parameter is passed by "non-const reference" and, therefore, the value may differ once the call completes.

I hear you on this and, normally, I'd have the same opinion. I don't like spooky action at a distance. But the point of *this* exercise (not a general one) is to make the glib API safer; we are being dictated to by the existing glib API and have to work with that.

Revision history for this message

James Henstridge (jamesh) wrote on 2017-03-22:

#

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.

Revision history for this message

James Henstridge (jamesh) wrote on 2017-03-22:

#

Never mind. I misunderstood what the purpose of the _uptr member was. It might have been a bit simpler to just use "_uptr.reset(ptr)" in the destructor though.

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-22:

#

OK, I'll live with the Assigner temporary at the call site. I used James's suggestions to do this with templates and find the the code more readable that way. Feel free to re-use (or not) as you see fit.

http://pastebin.ubuntu.com/24227239/

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

James, FYI doing the reset causes the deleter function to become invalid as far as I can tell. I don't quite understand this myself.

If you do e.g.:

unique_ptr<Foo, MyDeleter> o(nullptr, MyDeleter());
o.reset(new_foo()); // unique_ptr doesn't let you pass the deleter again here, presumably intentionally.

then unique_ptr causes a segfault on destruction.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

@michi, aha! the thing I was missing was prototyping a templated class. For some reason I didn't know you could do this.

Revision history for this message

James Henstridge (jamesh) wrote on 2017-03-22:

#

Pete: IIRC, you can only set the deleter for a unique_ptr through its constructor. The reset() method changes the value but keeps the same deleter.

I suspect the problem you might have had is that in its current form, a default constructed GLibDeleter<type,delete_func> will segfault when used: the "_d" function pointer member won't have been initialised, so crashes when you try to call it.

See http://paste.ubuntu.com/24228221/ for example.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

@michi thanks very much for spending the time on that implementation. I've tweaked it ever so slightly (and also prodded it quite a bit to try and understand all the tricks in it).

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

I'm just checking it for backwards compatibility with the only user at the moment, which is ubuntu-app-launch.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

Following up on James's comment, though. The changed deleter type doesn't make unique_ptr get upset any more. So I've changed the call to unique_ptr to a .reset.

Revision history for this message

James Henstridge (jamesh) wrote on 2017-03-22:

#

One last suggestion: rather than having the user pick an assigner class, how about using a function to create assigner object, so that the type can be inferred?

Taking code from the tests, something like:

gcharUPtr name;
g_object_get(obj, "name", assign_to(name), nullptr);

GErrorUPtr error;
EXPECT_FALSE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_to(error)));

This should be pretty easy if all the assigner classes are instantiations of the one template.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

Seems that was a mistake. As the original unique_ptr may not have been initialized with the correct deleter, so for unique_ptr we need to use unique_glib and move.

For shared_ptr this isn't the case, though.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-22:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/159/rebuild

review: Approve (continuous-integration)

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

Have split out a shared_ptr and a unique_ptr version of the assigner. Feel free to tell me how to reduce the code complication between these two templates :)

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

code *duplication*

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

James, if that works will try and get it in now.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

The challenge there, of course, is I will probably have to make the types movable so they can exit the function.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

Okay, I've implemented James' suggestion now

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

The only thing really bothering me now is the dual implementations of shared and unique assigners. If you could conditionally compile different stuff into the deleter depending of if the template argument was a uptr or sptr then you could collapse this down. You'd also be able to collapse the two assign_sptr and assign_uptr methods at the same time.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-22:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/160/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-03-22:

#

> The only thing really bothering me now is the dual implementations of shared
> and unique assigners. If you could conditionally compile different stuff into
> the deleter depending of if the template argument was a uptr or sptr then you
> could collapse this down. You'd also be able to collapse the two assign_sptr
> and assign_uptr methods at the same time.

Function overloading?

    ~GObjectPtrAssigner()
    {
        if (_ptr)
        {
           do_stuff(_smart, _ptr);
        }
    }

    void do_stuff(std::shared_ptr<TypeName>& _smart, element* _ptr)
    {
       _smart.reset(_ptr, GlibDeleter<typename S::element_type>());
    }

    void do_stuff(std::unique_ptr<TypeName>& _smart, element* _ptr)
    {
        _smart.reset(_ptr, GObjectDeleter());
    }

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-03-22:

#

Oops, didn't mean to vote

review: Abstain

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-22:

#

No worries. That's a great idea. I'll see if I can get it working.

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-23:

#

> Function overloading?

This works, but it means that each instance has two do_stuff() member functions, only one of which will ever be called, and the other one is dead code. (Admittedly, very little code.)

Instead, we can use SFINAE:

    ~GlibPtrAssigner()
    {
        release<SP>(smart_ptr_, ptr_);
    }

private:
// Instantiate exactly one of the two release() methods below, depending
// on whether we were instantiated with a shared_ptr or a unique_ptr.

    template<typename T>
    typename std::enable_if<
        std::is_same<T, std::shared_ptr<typename T::element_type>>::value,
        void
    >::type
    release(std::shared_ptr<ElementType>& smart, ElementType* p)
    {
        smart.reset(p, GlibDeleter<typename T::element_type>());
    }

    template<typename T>
    typename std::enable_if<
        std::is_same<T, std::unique_ptr<typename T::element_type, typename T::deleter_type>>::value,
        void
    >::type
    release(std::unique_ptr<ElementType, GlibDeleter<typename T::element_type>>& smart, ElementType* p)
    {
        smart.reset(p);
    }

Pete, I've pushed this change here:

https://code.launchpad.net/~unity-api-team/unity-api/add-glib-assigner-class

It's now possible to use the same glib_assign method, whether the argument is a shared_ptr or a unique_ptr:

EXPECT_FALSE(g_key_file_get_boolean(gkf.get(), "group", "key", glib_assign(error)));

Revision history for this message

James Henstridge (jamesh) wrote on 2017-03-23:

#

I had a little play with your branch, and made a few cleanups in this diff:

http://paste.ubuntu.com/24232796/

Primarily:

1. move some of the detail types to the internal namespace
2. rename glib_assign_uptr and glib_assign_sptr to glib_assign()
3. merge GlibAssigner{UPtr,SPtr} implementations

There are some functions that are returning internal types, but they are either (a) types with public aliases, or (b) types that the caller shouldn't be referencing directly.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-23:

#

Argh! Too much help now! James and Michi's changes conflict!

(But seriously, thanks for everyone's help)

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-23:

#

Relax! Just look through both of them. Overall, James's approach is more elegant, I think. In particular, it doesn't need the SFINAE hackery.

Some of the comments I wrote would be nice to keep, I think.

One other thought: We should mark all methods that can't throw (I think that's like all of them) as noexcept.

And doing a pass for coding style would be good. (ptr_ instead _of _ptr, and the alignment of the member initialisers.)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-03-23:

#

> > Function overloading?
>
> This works, but it means that each instance has two do_stuff() member
> functions, only one of which will ever be called, and the other one is dead
> code. (Admittedly, very little code.)

A class template only instantiates functions that are used.

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-23:

#

Aargh... I live and learn. Thanks for that!

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-23:

#

:) I'm only being dramatic.

I've pulled michi's branch for the style changes, and I'm going to weave James' implementation of the destructor in, as it's definitely much simpler!

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-23:

#

I'm not totally convinced by making all the types internal approach. But other than that this is looking very good.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-23:

#

For formatting style I'm just using Eclipse's BSD/Allman mode, FYI.

Revision history for this message

Pete Woods (pete-woods) wrote on 2017-03-23:

#

Right, this should be in a state for a final review now, unless we want to argue about the internal namespace thing.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-23:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/161/rebuild

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-23:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/162/rebuild

review: Approve (continuous-integration)

Revision history for this message

Michi Henning (michihenning) wrote on 2017-03-23:

#

This looks good to me. Thanks for pushing this through, Pete!

review: Approve

Status:	Merged
Approved by:	Pete Woods on 2017-04-04
Approved revision:	296
Merged at revision:	281
Proposed branch:	lp:~pete-woods/unity-api/add-glib-assigner-class
Merge into:	lp:unity-api
Prerequisite:	lp:~pete-woods/unity-api/throw-for-unowned-types-lp1672657
Diff against target:	668 lines (+447/-43) 5 files modified debian/changelog (+6/-0) include/unity/util/GObjectMemory.h (+67/-6) include/unity/util/GlibMemory.h (+142/-24) test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp (+104/-13) test/gtest/unity/util/GlibMemory/GlibMemory_test.cpp (+128/-0)
To merge this branch:	bzr merge lp:~pete-woods/unity-api/add-glib-assigner-class
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Michi Henning (community)			Approve on 2017-03-23
Unity8 CI Bot	continuous-integration		Approve on 2017-03-23
Alan Griffiths			Abstain on 2017-03-22
Charles Kerr (community)		2017-03-20	Approve on 2017-03-20
Review via email: mp+320375@code.launchpad.net

Unity API

Merge lp:~pete-woods/unity-api/add-glib-assigner-class into lp:unity-api

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'debian/changelog'
 --- debian/changelog	2017-03-17 11:08:20 +0000
 +++ debian/changelog	2017-04-04 09:33:34 +0000
@@ -1,3 +1,9 @@
++unity-api (8.7-0ubuntu1) UNRELEASED; urgency=medium
++
++  * Add Glib and GObject Assigner helpers.
++
++ -- Pete <pete.woods@canonical.com>  Fri, 24 Mar 2017 10:25:45 +0000
++
  unity-api (8.6+17.04.20170317-0ubuntu1) zesty; urgency=medium
    [ Michael Zanetti ]
 === modified file 'include/unity/util/GObjectMemory.h'
 --- include/unity/util/GObjectMemory.h	2017-03-17 10:20:10 +0000
 +++ include/unity/util/GObjectMemory.h	2017-04-04 09:33:34 +0000
@@ -57,7 +57,7 @@
   */
  struct GObjectDeleter
+ {
--    void operator()(gpointer ptr)
++    void operator()(gpointer ptr) noexcept
+     {
          if (G_IS_OBJECT(ptr))
+         {
@@ -66,6 +66,51 @@
+     }
  };
++template<typename T> using GObjectSPtr = std::shared_ptr<T>;
++template<typename T> using GObjectUPtr = std::unique_ptr<T, GObjectDeleter>;
++
++namespace internal
++{
++
++template<typename SP>
++class GObjectAssigner
++{
++public:
++    typedef typename SP::element_type ElementType;
++
++    GObjectAssigner(SP& smart_ptr) noexcept:
++            smart_ptr_(smart_ptr)
++    {
++    }
++
++    GObjectAssigner(const GObjectAssigner& other) = delete;
++
++    GObjectAssigner(GObjectAssigner&& other) noexcept:
++            ptr_(other.ptr_), smart_ptr_(other.smart_ptr_)
++    {
++        other.ptr_ = nullptr;
++    }
++
++    ~GObjectAssigner() noexcept
++    {
++        smart_ptr_ = SP(ptr_, GObjectDeleter());
++    }
++
++    GObjectAssigner& operator=(const GObjectAssigner& other) = delete;
++
++    operator ElementType**() noexcept
++    {
++        return &ptr_;
++    }
++
++private:
++    ElementType* ptr_ = nullptr;
++
++    SP& smart_ptr_;
++};
++
++}
++
  /**
   \brief Helper method to wrap a unique_ptr around an existing GObject.
@@ -79,11 +124,11 @@
   \endcode
   */
  template<typename T>
--inline std::unique_ptr<T, GObjectDeleter> unique_gobject(T* ptr)
++inline GObjectUPtr<T> unique_gobject(T* ptr)
+ {
      check_floating_gobject(ptr);
      GObjectDeleter d;
--    return std::unique_ptr<T, GObjectDeleter>(ptr, d);
++    return GObjectUPtr<T>(ptr, d);
+ }
  /**
@@ -99,11 +144,11 @@
   \endcode
   */
  template<typename T>
--inline std::shared_ptr<T> share_gobject(T* ptr)
++inline GObjectSPtr<T> share_gobject(T* ptr)
+ {
      check_floating_gobject(ptr);
      GObjectDeleter d;
--    return std::shared_ptr<T>(ptr, d);
++    return GObjectSPtr<T>(ptr, d);
+ }
  /**
@@ -117,7 +162,7 @@
   \endcode
   */
  template<typename T, typename ... Args>
--inline std::unique_ptr<T, GObjectDeleter> make_gobject(GType object_type, const gchar *first_property_name, Args&&... args)
++inline GObjectUPtr<T> make_gobject(GType object_type, const gchar *first_property_name, Args&&... args) noexcept
+ {
      gpointer ptr = g_object_new(object_type, first_property_name, std::forward<Args>(args)...);
      if (G_IS_OBJECT(ptr) && g_object_is_floating(ptr))
@@ -127,6 +172,22 @@
      return unique_gobject(G_TYPE_CHECK_INSTANCE_CAST(ptr, object_type, T));
+ }
++/**
++ \brief Helper method to take ownership of GObjects assigned from a reference.
++
++ Example:
++ \code{.cpp}
++ GObjectUPtr<FooBar> o;
++ method_that_assigns_a_foobar(assign_gobject(o));
++ \endcode
++ */
++template<typename SP>
++inline internal::GObjectAssigner<SP> assign_gobject(SP& smart_ptr) noexcept
++{
++    return internal::GObjectAssigner<SP>(smart_ptr);
++}
++
++
  }  // namespace until
  }  // namespace unity
 === modified file 'include/unity/util/GlibMemory.h'
 --- include/unity/util/GlibMemory.h	2017-02-23 11:18:54 +0000
 +++ include/unity/util/GlibMemory.h	2017-04-04 09:33:34 +0000
@@ -14,6 +14,8 @@
   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+  *
   * Authored by: Pete Woods <pete.woods@canonical.com>
++ *              Michi Henning <michi.henning@canonical.com>
++ *              James Henstridge <james.henstridge@canonical.com>
   */
  #ifndef UNITY_UTIL_GLIBMEMORY_H
@@ -28,45 +30,161 @@
  namespace util
+ {
--template<typename T, typename D>
--struct GlibDeleter
--{
--    D _d;
--
--    void operator()(T* ptr)
--    {
--        if (ptr != nullptr)
--        {
--            _d(ptr);
--        }
--    }
++namespace internal
++{
++
++template<typename T> struct GlibDeleter;
++template<typename T> using GlibSPtr = std::shared_ptr<T>;
++template<typename T> using GlibUPtr = std::unique_ptr<T, GlibDeleter<T>>;
++
++/**
++ * \brief Adapter class to assign to smart pointers from functions that take a reference.
++ *
++ * Adapter class that allows passing a shared_ptr or unique_ptr where glib
++ * expects a parameter of type ElementType** (such as GError**), by providing
++ * a default conversion operator to ElementType**. This allows the glib method
++ * to assign to the ptr_ member. From the destructor, we assign to the
++ * provided smart pointer.
++ */
++template<typename SP>
++class GlibAssigner
++{
++public:
++    typedef typename SP::element_type ElementType;
++
++    GlibAssigner(SP& smart_ptr) noexcept :
++            smart_ptr_(smart_ptr)
++    {
++    }
++
++    GlibAssigner(const GlibAssigner& other) = delete;
++
++    GlibAssigner(GlibAssigner&& other) noexcept:
++            ptr_(other.ptr_), smart_ptr_(other.smart_ptr_)
++    {
++        other.ptr_ = nullptr;
++    }
++
++    ~GlibAssigner() noexcept
++    {
++        smart_ptr_ = SP(ptr_, GlibDeleter<ElementType>());
++    }
++
++    GlibAssigner& operator=(const GlibAssigner& other) = delete;
++
++    operator ElementType**() noexcept
++    {
++        return &ptr_;
++    }
++
++private:
++    ElementType* ptr_ = nullptr;
++
++    SP& smart_ptr_;
  };
++}
++
++#define UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(TypeName, func) \
++using TypeName##Deleter = internal::GlibDeleter<TypeName>; \
++using TypeName##SPtr = internal::GlibSPtr<TypeName>; \
++using TypeName##UPtr = internal::GlibUPtr<TypeName>; \
++namespace internal \
++{ \
++template<> struct GlibDeleter<TypeName> \
++{ \
++    void operator()(TypeName* ptr) noexcept \
++    { \
++        if (ptr) \
++        { \
++            ::func(ptr); \
++        } \
++    } \
++}; \
++}
++
++/**
++ \brief Helper method to wrap a shared_ptr around a Glib type.
++
++ Example:
++ \code{.cpp}
++ auto gkf = shared_glib(g_key_file_new());
++ \endcode
++ */
++template<typename T>
++inline internal::GlibSPtr<T> share_glib(T* ptr) noexcept
++{
++    return internal::GlibSPtr<T>(ptr, internal::GlibDeleter<T>());
++}
++
++/**
++ \brief Helper method to wrap a unique_ptr around a Glib type.
++
++ Example:
++ \code{.cpp}
++ auto gkf = unique_glib(g_key_file_new());
++ \endcode
++ */
++template<typename T>
++inline internal::GlibUPtr<T> unique_glib(T* ptr) noexcept
++{
++    return internal::GlibUPtr<T>(ptr, internal::GlibDeleter<T>());
++}
++
++/**
++ \brief Helper method to take ownership of glib types assigned from a reference.
++
++ Example:
++ \code{.cpp}
++ GErrorUPtr error;
++ if (!g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)))
++ {
++     std::cerr << error->message << std::endl;
++     throw some_exception();
++ }
++ \endcode
++
++ Another example:
++ \code{.cpp}
++ gcharUPtr name;
++ g_object_get(obj, "name", assign_glib(name), nullptr);
++ \endcode
++ */
++template<typename SP>
++inline internal::GlibAssigner<SP> assign_glib(SP& smart_ptr) noexcept
++{
++    return internal::GlibAssigner<SP>(smart_ptr);
++}
++
++/**
++ * Below here is some hackery to extract the matching deleters for all built in glib types.
++ */
  #pragma push_macro("G_DEFINE_AUTOPTR_CLEANUP_FUNC")
  #undef G_DEFINE_AUTOPTR_CLEANUP_FUNC
--#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
--typedef GlibDeleter<TypeName, decltype(&func)> TypeName##Deleter; \
--typedef std::shared_ptr<TypeName> TypeName##SPtr; \
--inline TypeName##SPtr share_glib(TypeName* ptr) \
--{ \
--    return TypeName##SPtr(ptr, TypeName##Deleter{&func}); \
--} \
--typedef std::unique_ptr<TypeName, TypeName##Deleter> TypeName##UPtr; \
--inline TypeName##UPtr unique_glib(TypeName* ptr) \
--{ \
--    return TypeName##UPtr(ptr, TypeName##Deleter{&func}); \
--}
++#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(TypeName, func)
  #pragma push_macro("G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC")
  #undef G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC
  #define G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(TypeName, func)
++#pragma push_macro("G_DEFINE_AUTO_CLEANUP_FREE_FUNC")
++#undef G_DEFINE_AUTO_CLEANUP_FREE_FUNC
++#define G_DEFINE_AUTO_CLEANUP_FREE_FUNC(TypeName, func, none)
++
  #define __GLIB_H_INSIDE__
  #include <glib/glib-autocleanups.h>
  #undef __GLIB_H_INSIDE__
  #pragma pop_macro("G_DEFINE_AUTOPTR_CLEANUP_FUNC")
  #pragma pop_macro("G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC")
++#pragma pop_macro("G_DEFINE_AUTO_CLEANUP_FREE_FUNC")
++
++/**
++ * Manually add extra definitions for gchar* and gchar**
++ */
++UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(gchar, g_free)
++typedef gchar* gcharv;
++UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(gcharv, g_strfreev)
  }  // namespace until
 === modified file 'test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp'
 --- test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp	2017-03-17 10:20:10 +0000
 +++ test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp	2017-04-04 09:33:34 +0000
@@ -17,8 +17,8 @@
   *              Pete Woods <pete.woods@canonical.com>
   */
++#include <unity/util/GlibMemory.h>
  #include <unity/util/GObjectMemory.h>
--#include <glib-object.h>
  #include <gtest/gtest.h>
  #include <list>
  #include <string>
@@ -46,8 +46,14 @@
  FooBar *foo_bar_new();
++void foo_bar_assigner(FooBar** in);
++
  FooBar *foo_bar_new_full(const gchar* const name, guint id);
++void foo_bar_assigner_full(const gchar* const name, guint id, FooBar** in);
++
++void foo_bar_assigner_null(FooBar** in);
++
  G_END_DECLS
  // private implementation
@@ -167,6 +173,30 @@
      return FOO_BAR(g_object_new(FOO_TYPE_BAR, "name", name, "id", id, NULL));
+ }
++void foo_bar_assigner(FooBar** in)
++{
++    if (in != nullptr)
++    {
++        *in = foo_bar_new();
++    }
++}
++
++void foo_bar_assigner_full(const gchar* const name, guint id, FooBar** in)
++{
++    if (in != nullptr)
++    {
++        *in = foo_bar_new_full(name, id);
++    }
++}
++
++void foo_bar_assigner_null(FooBar** in)
++{
++    if (in != nullptr)
++    {
++        *in = nullptr;
++    }
++}
++
  //
  // Test cases
  //
@@ -337,12 +367,12 @@
+ }
  TEST_F(GObjectMemoryTest, sizeoftest) {
--    EXPECT_EQ(sizeof(FooBar*), sizeof(unique_ptr<FooBar>));
++    EXPECT_EQ(sizeof(FooBar*), sizeof(GObjectUPtr<FooBar>));
+ }
  TEST_F(GObjectMemoryTest, hash)
+ {
--    unordered_set<shared_ptr<FooBar>> s;
++    unordered_set<GObjectSPtr<FooBar>> s;
      auto a = share_gobject(foo_bar_new());
      auto b = share_gobject(foo_bar_new());
      auto c = share_gobject(foo_bar_new());
@@ -403,10 +433,57 @@
      EXPECT_EQ(list<Deleted>({{"c", 3}, {"b", 2}, {"a", 1}}), DELETED_OBJECTS);
+ }
--TEST_F(GObjectMemoryTest, foo)
--{
--    {
--        shared_ptr<FooBar> s;
++TEST_F(GObjectMemoryTest, uptrAssignerDeletesGObjects)
++{
++    {
++        GObjectUPtr<FooBar> a, b;
++        foo_bar_assigner_full("a", 1, assign_gobject(a));
++        foo_bar_assigner_full("b", 2, assign_gobject(b));
++    }
++    EXPECT_EQ(list<Deleted>({{"b", 2}, {"a", 1}}), DELETED_OBJECTS);
++}
++
++TEST_F(GObjectMemoryTest, sptrAssignerDeletesGObjects)
++{
++    {
++        GObjectSPtr<FooBar> a, b, c;
++        foo_bar_assigner_full("a", 3, assign_gobject(a));
++        foo_bar_assigner_full("b", 4, assign_gobject(b));
++        foo_bar_assigner_full("c", 5, assign_gobject(c));
++    }
++    EXPECT_EQ(list<Deleted>({{"c", 5}, {"b", 4}, {"a", 3}}), DELETED_OBJECTS);
++}
++
++TEST_F(GObjectMemoryTest, uptrAssignerAssignsNull)
++{
++    {
++       GObjectUPtr<FooBar> o;
++       foo_bar_assigner_full("o", 1, assign_gobject(o));
++       ASSERT_TRUE(bool(o));
++       foo_bar_assigner_null(assign_gobject(o));
++       ASSERT_FALSE(bool(o));
++
++    }
++    EXPECT_EQ(list<Deleted>({{"o", 1}}), DELETED_OBJECTS);
++}
++
++TEST_F(GObjectMemoryTest, sptrAssignerAssignsNull)
++{
++    {
++       GObjectSPtr<FooBar> o;
++       foo_bar_assigner_full("o", 1, assign_gobject(o));
++       ASSERT_TRUE(bool(o));
++       foo_bar_assigner_null(assign_gobject(o));
++       ASSERT_FALSE(bool(o));
++
++    }
++    EXPECT_EQ(list<Deleted>({{"o", 1}}), DELETED_OBJECTS);
++}
++
++TEST_F(GObjectMemoryTest, moveUptrSptr)
++{
++    {
++        GObjectSPtr<FooBar> s;
          auto u = unique_gobject(foo_bar_new_full("hi", 6));
          s = move(u);
          // unique instance has been stolen from
@@ -414,7 +491,7 @@
+     }
      EXPECT_EQ(list<Deleted>({{"hi", 6}}), DELETED_OBJECTS);
+     {
--        shared_ptr<FooBar> s(unique_gobject(foo_bar_new_full("bye", 7)));
++        GObjectSPtr<FooBar> s(unique_gobject(foo_bar_new_full("bye", 7)));
+     }
      EXPECT_EQ(list<Deleted>({{"hi", 6}, {"bye", 7}}), DELETED_OBJECTS);
+ }
@@ -431,14 +508,12 @@
       */
      static void checkProperties(gpointer obj, const char* expectedName, guint expectedId)
+     {
--        char* name = NULL;
++        gcharUPtr name;
          guint id = 0;
--        g_object_get(obj, "name", &name, "id", &id, NULL);
--        EXPECT_STREQ(expectedName, name);
++        g_object_get(obj, "name", assign_glib(name), "id", &id, nullptr);
++        EXPECT_STREQ(expectedName, name.get());
          EXPECT_EQ(expectedId, id);
--
--        g_free(name);
+     }
  };
@@ -471,6 +546,22 @@
      checkProperties(obj.get(), p.first, p.second);
+ }
++TEST_P(GObjectMemoryMakeHelperMethodsTest, assign_foo_bar_uptr_assigner)
++{
++    auto p = GetParam();
++    GObjectUPtr<FooBar> obj;
++    foo_bar_assigner_full(p.first, p.second, assign_gobject(obj));
++    checkProperties(obj.get(), p.first, p.second);
++}
++
++TEST_P(GObjectMemoryMakeHelperMethodsTest, assign_foo_bar_sptr_assigner)
++{
++    auto p = GetParam();
++    GObjectSPtr<FooBar> obj;
++    foo_bar_assigner_full(p.first, p.second, assign_gobject(obj));
++    checkProperties(obj.get(), p.first, p.second);
++}
++
  INSTANTIATE_TEST_CASE_P(BunchOfNames,
                          GObjectMemoryMakeHelperMethodsTest,
                          ::testing::Values(GObjectMemoryMakeSharedTestParam{"meeny", 1},
 === modified file 'test/gtest/unity/util/GlibMemory/GlibMemory_test.cpp'
 --- test/gtest/unity/util/GlibMemory/GlibMemory_test.cpp	2017-02-23 11:18:54 +0000
 +++ test/gtest/unity/util/GlibMemory/GlibMemory_test.cpp	2017-04-04 09:33:34 +0000
@@ -89,6 +89,30 @@
+     {
          return share_glib(g_hash_table_new_full(g_str_hash, g_str_equal, deleteKey, deleteValue));
+     }
++
++    static void assignGChar(gchar** in)
++    {
++        if (in != nullptr)
++        {
++            *in = g_strdup("hi");
++        }
++    }
++
++    static void assignGError(GError** in)
++    {
++        if (in != nullptr)
++        {
++            *in = g_error_new_literal(g_key_file_error_quark(), 1, "hello");
++        }
++    }
++
++    static void assignEmptyGError(GError** in)
++    {
++        if (in != nullptr)
++        {
++            *in = nullptr;
++        }
++    }
  };
  TEST_F(GlibMemoryTest, Unique)
@@ -147,4 +171,108 @@
+     }
+ }
++TEST_F(GlibMemoryTest, UPtrAssigner)
++{
++    auto gkf = unique_glib(g_key_file_new());
++
++    {
++        GErrorUPtr error;
++        EXPECT_FALSE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
++        ASSERT_TRUE(bool(error));
++        EXPECT_EQ(G_KEY_FILE_ERROR_GROUP_NOT_FOUND, error->code);
++    }
++
++    g_key_file_set_boolean(gkf.get(), "group", "key", TRUE);
++
++    {
++        GErrorUPtr error;
++        EXPECT_TRUE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
++        EXPECT_FALSE(bool(error));
++    }
++
++    {
++        gcharUPtr str;
++        assignGChar(assign_glib(str));
++        ASSERT_TRUE(bool(str));
++        EXPECT_STREQ("hi", str.get());
++    }
++
++    {
++       GErrorUPtr error;
++       assignGError(assign_glib(error));
++       ASSERT_TRUE(bool(error));
++       assignEmptyGError(assign_glib(error));
++       ASSERT_FALSE(bool(error));
++   }
++}
++
++TEST_F(GlibMemoryTest, SPtrAssigner)
++{
++    auto gkf = share_glib(g_key_file_new());
++
++    {
++        GErrorSPtr error;
++        EXPECT_FALSE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
++        ASSERT_TRUE(bool(error));
++        EXPECT_EQ(G_KEY_FILE_ERROR_GROUP_NOT_FOUND, error->code);
++    }
++
++    g_key_file_set_boolean(gkf.get(), "group", "key", TRUE);
++
++    {
++        GErrorSPtr error;
++        EXPECT_TRUE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
++        EXPECT_FALSE(bool(error));
++    }
++
++    {
++        gcharSPtr str;
++        assignGChar(assign_glib(str));
++        ASSERT_TRUE(bool(str));
++        EXPECT_STREQ("hi", str.get());
++    }
++
++    {
++       GErrorSPtr error;
++       assignGError(assign_glib(error));
++       ASSERT_TRUE(bool(error));
++       assignEmptyGError(assign_glib(error));
++       ASSERT_FALSE(bool(error));
++   }
++}
++
++TEST_F(GlibMemoryTest, GCharUPtr)
++{
++    {
++        auto strv = unique_glib(g_strsplit("a b c", " ", 0));
++        ASSERT_TRUE(bool(strv));
++        EXPECT_STREQ("a", strv.get()[0]);
++        EXPECT_STREQ("b", strv.get()[1]);
++        EXPECT_STREQ("c", strv.get()[2]);
++    }
++
++    {
++        auto str = unique_glib(g_strdup("hello"));
++        ASSERT_TRUE(bool(str));
++        EXPECT_STREQ("hello", str.get());
++    }
++}
++
++TEST_F(GlibMemoryTest, GCharSPtr)
++{
++    {
++        auto strv = share_glib(g_strsplit("a b c", " ", 0));
++        ASSERT_TRUE(bool(strv));
++        EXPECT_STREQ("a", strv.get()[0]);
++        EXPECT_STREQ("b", strv.get()[1]);
++        EXPECT_STREQ("c", strv.get()[2]);
++    }
++
++    {
++        auto str = share_glib(g_strdup("hello"));
++        ASSERT_TRUE(bool(str));
++        EXPECT_STREQ("hello", str.get());
++    }
++}
++
+ }