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

Proposed by Pete Woods on 2017-03-20
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
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

Commit message

Add assigner class to support common GError usage pattern

Description of the change

Add assigner class to support common GError usage pattern as below:

GErrorUPtr error;
auto o = unique_glib(g_foobar(args..., GErrorAssigner(error)))
if (error)
{
    cerr << error->message << endl;
    return;
}

This avoids us having to manually free the error object, or having to mess around with an explicit temporary variable.

To post a comment you must log in.
Charles Kerr (charlesk) wrote :

LGTM.

review: Approve
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Michi Henning (michihenning) wrote :

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
Michi Henning (michihenning) wrote :

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.

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:279
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/156/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4579
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4607
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4434
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4434/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4434
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4434/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4434
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4434/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4434
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4434/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4434
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4434/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4434
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4434/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Pete Woods (pete-woods) wrote :

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);

Pete Woods (pete-woods) wrote :

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.

Michi Henning (michihenning) wrote :

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?

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:281
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/157/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4598
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4626
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4453
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4453/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4453
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4453/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4453
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4453/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4453
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4453/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4453
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4453/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4453
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4453/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Pete Woods (pete-woods) wrote :

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.

Michi Henning (michihenning) wrote :

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

Pete Woods (pete-woods) wrote :

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.

Michi Henning (michihenning) wrote :

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

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.

James Henstridge (jamesh) wrote :

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.

Michi Henning (michihenning) wrote :

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/

Pete Woods (pete-woods) wrote :

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.

Pete Woods (pete-woods) wrote :

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

James Henstridge (jamesh) wrote :

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.

Pete Woods (pete-woods) wrote :

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

Pete Woods (pete-woods) wrote :

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

Pete Woods (pete-woods) wrote :

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.

James Henstridge (jamesh) wrote :

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.

Pete Woods (pete-woods) wrote :

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.

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:282
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/159/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4611
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4639
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4466
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4466/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4466
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4466/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4466
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4466/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4466
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4466/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4466
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4466/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4466
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4466/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Pete Woods (pete-woods) wrote :

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 :)

Pete Woods (pete-woods) wrote :

code *duplication*

Pete Woods (pete-woods) wrote :

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

Pete Woods (pete-woods) wrote :

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

Pete Woods (pete-woods) wrote :

Okay, I've implemented James' suggestion now

Pete Woods (pete-woods) wrote :

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.

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:283
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/160/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4616
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4644
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4471
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4471/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4471
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4471/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4471
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4471/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4471
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4471/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4471
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4471/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4471
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4471/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

> 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
Alan Griffiths (alan-griffiths) wrote :

Oops, didn't mean to vote

review: Abstain
Pete Woods (pete-woods) wrote :

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

Michi Henning (michihenning) wrote :

> 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)));

James Henstridge (jamesh) wrote :

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.

Pete Woods (pete-woods) wrote :

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

(But seriously, thanks for everyone's help)

Michi Henning (michihenning) wrote :

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

Alan Griffiths (alan-griffiths) wrote :

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

Michi Henning (michihenning) wrote :

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

Pete Woods (pete-woods) wrote :

:) 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!

Pete Woods (pete-woods) wrote :

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

Pete Woods (pete-woods) wrote :

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

Pete Woods (pete-woods) wrote :

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

Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:287
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/161/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4628
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4656
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4483
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4483/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4483
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4483/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4483
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4483/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4483
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4483/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4483
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4483/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4483
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4483/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:288
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/162/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4629
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4657
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4484
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4484/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4484
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4484/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4484
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4484/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4484
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4484/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4484
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4484/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4484
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4484/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Michi Henning (michihenning) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2017-03-17 11:08:20 +0000
3+++ debian/changelog 2017-04-04 09:33:34 +0000
4@@ -1,3 +1,9 @@
5+unity-api (8.7-0ubuntu1) UNRELEASED; urgency=medium
6+
7+ * Add Glib and GObject Assigner helpers.
8+
9+ -- Pete <pete.woods@canonical.com> Fri, 24 Mar 2017 10:25:45 +0000
10+
11 unity-api (8.6+17.04.20170317-0ubuntu1) zesty; urgency=medium
12
13 [ Michael Zanetti ]
14
15=== modified file 'include/unity/util/GObjectMemory.h'
16--- include/unity/util/GObjectMemory.h 2017-03-17 10:20:10 +0000
17+++ include/unity/util/GObjectMemory.h 2017-04-04 09:33:34 +0000
18@@ -57,7 +57,7 @@
19 */
20 struct GObjectDeleter
21 {
22- void operator()(gpointer ptr)
23+ void operator()(gpointer ptr) noexcept
24 {
25 if (G_IS_OBJECT(ptr))
26 {
27@@ -66,6 +66,51 @@
28 }
29 };
30
31+template<typename T> using GObjectSPtr = std::shared_ptr<T>;
32+template<typename T> using GObjectUPtr = std::unique_ptr<T, GObjectDeleter>;
33+
34+namespace internal
35+{
36+
37+template<typename SP>
38+class GObjectAssigner
39+{
40+public:
41+ typedef typename SP::element_type ElementType;
42+
43+ GObjectAssigner(SP& smart_ptr) noexcept:
44+ smart_ptr_(smart_ptr)
45+ {
46+ }
47+
48+ GObjectAssigner(const GObjectAssigner& other) = delete;
49+
50+ GObjectAssigner(GObjectAssigner&& other) noexcept:
51+ ptr_(other.ptr_), smart_ptr_(other.smart_ptr_)
52+ {
53+ other.ptr_ = nullptr;
54+ }
55+
56+ ~GObjectAssigner() noexcept
57+ {
58+ smart_ptr_ = SP(ptr_, GObjectDeleter());
59+ }
60+
61+ GObjectAssigner& operator=(const GObjectAssigner& other) = delete;
62+
63+ operator ElementType**() noexcept
64+ {
65+ return &ptr_;
66+ }
67+
68+private:
69+ ElementType* ptr_ = nullptr;
70+
71+ SP& smart_ptr_;
72+};
73+
74+}
75+
76 /**
77 \brief Helper method to wrap a unique_ptr around an existing GObject.
78
79@@ -79,11 +124,11 @@
80 \endcode
81 */
82 template<typename T>
83-inline std::unique_ptr<T, GObjectDeleter> unique_gobject(T* ptr)
84+inline GObjectUPtr<T> unique_gobject(T* ptr)
85 {
86 check_floating_gobject(ptr);
87 GObjectDeleter d;
88- return std::unique_ptr<T, GObjectDeleter>(ptr, d);
89+ return GObjectUPtr<T>(ptr, d);
90 }
91
92 /**
93@@ -99,11 +144,11 @@
94 \endcode
95 */
96 template<typename T>
97-inline std::shared_ptr<T> share_gobject(T* ptr)
98+inline GObjectSPtr<T> share_gobject(T* ptr)
99 {
100 check_floating_gobject(ptr);
101 GObjectDeleter d;
102- return std::shared_ptr<T>(ptr, d);
103+ return GObjectSPtr<T>(ptr, d);
104 }
105
106 /**
107@@ -117,7 +162,7 @@
108 \endcode
109 */
110 template<typename T, typename ... Args>
111-inline std::unique_ptr<T, GObjectDeleter> make_gobject(GType object_type, const gchar *first_property_name, Args&&... args)
112+inline GObjectUPtr<T> make_gobject(GType object_type, const gchar *first_property_name, Args&&... args) noexcept
113 {
114 gpointer ptr = g_object_new(object_type, first_property_name, std::forward<Args>(args)...);
115 if (G_IS_OBJECT(ptr) && g_object_is_floating(ptr))
116@@ -127,6 +172,22 @@
117 return unique_gobject(G_TYPE_CHECK_INSTANCE_CAST(ptr, object_type, T));
118 }
119
120+/**
121+ \brief Helper method to take ownership of GObjects assigned from a reference.
122+
123+ Example:
124+ \code{.cpp}
125+ GObjectUPtr<FooBar> o;
126+ method_that_assigns_a_foobar(assign_gobject(o));
127+ \endcode
128+ */
129+template<typename SP>
130+inline internal::GObjectAssigner<SP> assign_gobject(SP& smart_ptr) noexcept
131+{
132+ return internal::GObjectAssigner<SP>(smart_ptr);
133+}
134+
135+
136 } // namespace until
137
138 } // namespace unity
139
140=== modified file 'include/unity/util/GlibMemory.h'
141--- include/unity/util/GlibMemory.h 2017-02-23 11:18:54 +0000
142+++ include/unity/util/GlibMemory.h 2017-04-04 09:33:34 +0000
143@@ -14,6 +14,8 @@
144 * along with this program. If not, see <http://www.gnu.org/licenses/>.
145 *
146 * Authored by: Pete Woods <pete.woods@canonical.com>
147+ * Michi Henning <michi.henning@canonical.com>
148+ * James Henstridge <james.henstridge@canonical.com>
149 */
150
151 #ifndef UNITY_UTIL_GLIBMEMORY_H
152@@ -28,45 +30,161 @@
153 namespace util
154 {
155
156-template<typename T, typename D>
157-struct GlibDeleter
158-{
159- D _d;
160-
161- void operator()(T* ptr)
162- {
163- if (ptr != nullptr)
164- {
165- _d(ptr);
166- }
167- }
168+namespace internal
169+{
170+
171+template<typename T> struct GlibDeleter;
172+template<typename T> using GlibSPtr = std::shared_ptr<T>;
173+template<typename T> using GlibUPtr = std::unique_ptr<T, GlibDeleter<T>>;
174+
175+/**
176+ * \brief Adapter class to assign to smart pointers from functions that take a reference.
177+ *
178+ * Adapter class that allows passing a shared_ptr or unique_ptr where glib
179+ * expects a parameter of type ElementType** (such as GError**), by providing
180+ * a default conversion operator to ElementType**. This allows the glib method
181+ * to assign to the ptr_ member. From the destructor, we assign to the
182+ * provided smart pointer.
183+ */
184+template<typename SP>
185+class GlibAssigner
186+{
187+public:
188+ typedef typename SP::element_type ElementType;
189+
190+ GlibAssigner(SP& smart_ptr) noexcept :
191+ smart_ptr_(smart_ptr)
192+ {
193+ }
194+
195+ GlibAssigner(const GlibAssigner& other) = delete;
196+
197+ GlibAssigner(GlibAssigner&& other) noexcept:
198+ ptr_(other.ptr_), smart_ptr_(other.smart_ptr_)
199+ {
200+ other.ptr_ = nullptr;
201+ }
202+
203+ ~GlibAssigner() noexcept
204+ {
205+ smart_ptr_ = SP(ptr_, GlibDeleter<ElementType>());
206+ }
207+
208+ GlibAssigner& operator=(const GlibAssigner& other) = delete;
209+
210+ operator ElementType**() noexcept
211+ {
212+ return &ptr_;
213+ }
214+
215+private:
216+ ElementType* ptr_ = nullptr;
217+
218+ SP& smart_ptr_;
219 };
220
221+}
222+
223+#define UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(TypeName, func) \
224+using TypeName##Deleter = internal::GlibDeleter<TypeName>; \
225+using TypeName##SPtr = internal::GlibSPtr<TypeName>; \
226+using TypeName##UPtr = internal::GlibUPtr<TypeName>; \
227+namespace internal \
228+{ \
229+template<> struct GlibDeleter<TypeName> \
230+{ \
231+ void operator()(TypeName* ptr) noexcept \
232+ { \
233+ if (ptr) \
234+ { \
235+ ::func(ptr); \
236+ } \
237+ } \
238+}; \
239+}
240+
241+/**
242+ \brief Helper method to wrap a shared_ptr around a Glib type.
243+
244+ Example:
245+ \code{.cpp}
246+ auto gkf = shared_glib(g_key_file_new());
247+ \endcode
248+ */
249+template<typename T>
250+inline internal::GlibSPtr<T> share_glib(T* ptr) noexcept
251+{
252+ return internal::GlibSPtr<T>(ptr, internal::GlibDeleter<T>());
253+}
254+
255+/**
256+ \brief Helper method to wrap a unique_ptr around a Glib type.
257+
258+ Example:
259+ \code{.cpp}
260+ auto gkf = unique_glib(g_key_file_new());
261+ \endcode
262+ */
263+template<typename T>
264+inline internal::GlibUPtr<T> unique_glib(T* ptr) noexcept
265+{
266+ return internal::GlibUPtr<T>(ptr, internal::GlibDeleter<T>());
267+}
268+
269+/**
270+ \brief Helper method to take ownership of glib types assigned from a reference.
271+
272+ Example:
273+ \code{.cpp}
274+ GErrorUPtr error;
275+ if (!g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)))
276+ {
277+ std::cerr << error->message << std::endl;
278+ throw some_exception();
279+ }
280+ \endcode
281+
282+ Another example:
283+ \code{.cpp}
284+ gcharUPtr name;
285+ g_object_get(obj, "name", assign_glib(name), nullptr);
286+ \endcode
287+ */
288+template<typename SP>
289+inline internal::GlibAssigner<SP> assign_glib(SP& smart_ptr) noexcept
290+{
291+ return internal::GlibAssigner<SP>(smart_ptr);
292+}
293+
294+/**
295+ * Below here is some hackery to extract the matching deleters for all built in glib types.
296+ */
297 #pragma push_macro("G_DEFINE_AUTOPTR_CLEANUP_FUNC")
298 #undef G_DEFINE_AUTOPTR_CLEANUP_FUNC
299-#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
300-typedef GlibDeleter<TypeName, decltype(&func)> TypeName##Deleter; \
301-typedef std::shared_ptr<TypeName> TypeName##SPtr; \
302-inline TypeName##SPtr share_glib(TypeName* ptr) \
303-{ \
304- return TypeName##SPtr(ptr, TypeName##Deleter{&func}); \
305-} \
306-typedef std::unique_ptr<TypeName, TypeName##Deleter> TypeName##UPtr; \
307-inline TypeName##UPtr unique_glib(TypeName* ptr) \
308-{ \
309- return TypeName##UPtr(ptr, TypeName##Deleter{&func}); \
310-}
311+#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(TypeName, func)
312
313 #pragma push_macro("G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC")
314 #undef G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC
315 #define G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(TypeName, func)
316
317+#pragma push_macro("G_DEFINE_AUTO_CLEANUP_FREE_FUNC")
318+#undef G_DEFINE_AUTO_CLEANUP_FREE_FUNC
319+#define G_DEFINE_AUTO_CLEANUP_FREE_FUNC(TypeName, func, none)
320+
321 #define __GLIB_H_INSIDE__
322 #include <glib/glib-autocleanups.h>
323 #undef __GLIB_H_INSIDE__
324
325 #pragma pop_macro("G_DEFINE_AUTOPTR_CLEANUP_FUNC")
326 #pragma pop_macro("G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC")
327+#pragma pop_macro("G_DEFINE_AUTO_CLEANUP_FREE_FUNC")
328+
329+/**
330+ * Manually add extra definitions for gchar* and gchar**
331+ */
332+UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(gchar, g_free)
333+typedef gchar* gcharv;
334+UNITY_UTIL_DEFINE_GLIB_SMART_POINTERS(gcharv, g_strfreev)
335
336 } // namespace until
337
338
339=== modified file 'test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp'
340--- test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp 2017-03-17 10:20:10 +0000
341+++ test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp 2017-04-04 09:33:34 +0000
342@@ -17,8 +17,8 @@
343 * Pete Woods <pete.woods@canonical.com>
344 */
345
346+#include <unity/util/GlibMemory.h>
347 #include <unity/util/GObjectMemory.h>
348-#include <glib-object.h>
349 #include <gtest/gtest.h>
350 #include <list>
351 #include <string>
352@@ -46,8 +46,14 @@
353
354 FooBar *foo_bar_new();
355
356+void foo_bar_assigner(FooBar** in);
357+
358 FooBar *foo_bar_new_full(const gchar* const name, guint id);
359
360+void foo_bar_assigner_full(const gchar* const name, guint id, FooBar** in);
361+
362+void foo_bar_assigner_null(FooBar** in);
363+
364 G_END_DECLS
365
366 // private implementation
367@@ -167,6 +173,30 @@
368 return FOO_BAR(g_object_new(FOO_TYPE_BAR, "name", name, "id", id, NULL));
369 }
370
371+void foo_bar_assigner(FooBar** in)
372+{
373+ if (in != nullptr)
374+ {
375+ *in = foo_bar_new();
376+ }
377+}
378+
379+void foo_bar_assigner_full(const gchar* const name, guint id, FooBar** in)
380+{
381+ if (in != nullptr)
382+ {
383+ *in = foo_bar_new_full(name, id);
384+ }
385+}
386+
387+void foo_bar_assigner_null(FooBar** in)
388+{
389+ if (in != nullptr)
390+ {
391+ *in = nullptr;
392+ }
393+}
394+
395 //
396 // Test cases
397 //
398@@ -337,12 +367,12 @@
399 }
400
401 TEST_F(GObjectMemoryTest, sizeoftest) {
402- EXPECT_EQ(sizeof(FooBar*), sizeof(unique_ptr<FooBar>));
403+ EXPECT_EQ(sizeof(FooBar*), sizeof(GObjectUPtr<FooBar>));
404 }
405
406 TEST_F(GObjectMemoryTest, hash)
407 {
408- unordered_set<shared_ptr<FooBar>> s;
409+ unordered_set<GObjectSPtr<FooBar>> s;
410 auto a = share_gobject(foo_bar_new());
411 auto b = share_gobject(foo_bar_new());
412 auto c = share_gobject(foo_bar_new());
413@@ -403,10 +433,57 @@
414 EXPECT_EQ(list<Deleted>({{"c", 3}, {"b", 2}, {"a", 1}}), DELETED_OBJECTS);
415 }
416
417-TEST_F(GObjectMemoryTest, foo)
418-{
419- {
420- shared_ptr<FooBar> s;
421+TEST_F(GObjectMemoryTest, uptrAssignerDeletesGObjects)
422+{
423+ {
424+ GObjectUPtr<FooBar> a, b;
425+ foo_bar_assigner_full("a", 1, assign_gobject(a));
426+ foo_bar_assigner_full("b", 2, assign_gobject(b));
427+ }
428+ EXPECT_EQ(list<Deleted>({{"b", 2}, {"a", 1}}), DELETED_OBJECTS);
429+}
430+
431+TEST_F(GObjectMemoryTest, sptrAssignerDeletesGObjects)
432+{
433+ {
434+ GObjectSPtr<FooBar> a, b, c;
435+ foo_bar_assigner_full("a", 3, assign_gobject(a));
436+ foo_bar_assigner_full("b", 4, assign_gobject(b));
437+ foo_bar_assigner_full("c", 5, assign_gobject(c));
438+ }
439+ EXPECT_EQ(list<Deleted>({{"c", 5}, {"b", 4}, {"a", 3}}), DELETED_OBJECTS);
440+}
441+
442+TEST_F(GObjectMemoryTest, uptrAssignerAssignsNull)
443+{
444+ {
445+ GObjectUPtr<FooBar> o;
446+ foo_bar_assigner_full("o", 1, assign_gobject(o));
447+ ASSERT_TRUE(bool(o));
448+ foo_bar_assigner_null(assign_gobject(o));
449+ ASSERT_FALSE(bool(o));
450+
451+ }
452+ EXPECT_EQ(list<Deleted>({{"o", 1}}), DELETED_OBJECTS);
453+}
454+
455+TEST_F(GObjectMemoryTest, sptrAssignerAssignsNull)
456+{
457+ {
458+ GObjectSPtr<FooBar> o;
459+ foo_bar_assigner_full("o", 1, assign_gobject(o));
460+ ASSERT_TRUE(bool(o));
461+ foo_bar_assigner_null(assign_gobject(o));
462+ ASSERT_FALSE(bool(o));
463+
464+ }
465+ EXPECT_EQ(list<Deleted>({{"o", 1}}), DELETED_OBJECTS);
466+}
467+
468+TEST_F(GObjectMemoryTest, moveUptrSptr)
469+{
470+ {
471+ GObjectSPtr<FooBar> s;
472 auto u = unique_gobject(foo_bar_new_full("hi", 6));
473 s = move(u);
474 // unique instance has been stolen from
475@@ -414,7 +491,7 @@
476 }
477 EXPECT_EQ(list<Deleted>({{"hi", 6}}), DELETED_OBJECTS);
478 {
479- shared_ptr<FooBar> s(unique_gobject(foo_bar_new_full("bye", 7)));
480+ GObjectSPtr<FooBar> s(unique_gobject(foo_bar_new_full("bye", 7)));
481 }
482 EXPECT_EQ(list<Deleted>({{"hi", 6}, {"bye", 7}}), DELETED_OBJECTS);
483 }
484@@ -431,14 +508,12 @@
485 */
486 static void checkProperties(gpointer obj, const char* expectedName, guint expectedId)
487 {
488- char* name = NULL;
489+ gcharUPtr name;
490 guint id = 0;
491
492- g_object_get(obj, "name", &name, "id", &id, NULL);
493- EXPECT_STREQ(expectedName, name);
494+ g_object_get(obj, "name", assign_glib(name), "id", &id, nullptr);
495+ EXPECT_STREQ(expectedName, name.get());
496 EXPECT_EQ(expectedId, id);
497-
498- g_free(name);
499 }
500 };
501
502@@ -471,6 +546,22 @@
503 checkProperties(obj.get(), p.first, p.second);
504 }
505
506+TEST_P(GObjectMemoryMakeHelperMethodsTest, assign_foo_bar_uptr_assigner)
507+{
508+ auto p = GetParam();
509+ GObjectUPtr<FooBar> obj;
510+ foo_bar_assigner_full(p.first, p.second, assign_gobject(obj));
511+ checkProperties(obj.get(), p.first, p.second);
512+}
513+
514+TEST_P(GObjectMemoryMakeHelperMethodsTest, assign_foo_bar_sptr_assigner)
515+{
516+ auto p = GetParam();
517+ GObjectSPtr<FooBar> obj;
518+ foo_bar_assigner_full(p.first, p.second, assign_gobject(obj));
519+ checkProperties(obj.get(), p.first, p.second);
520+}
521+
522 INSTANTIATE_TEST_CASE_P(BunchOfNames,
523 GObjectMemoryMakeHelperMethodsTest,
524 ::testing::Values(GObjectMemoryMakeSharedTestParam{"meeny", 1},
525
526=== modified file 'test/gtest/unity/util/GlibMemory/GlibMemory_test.cpp'
527--- test/gtest/unity/util/GlibMemory/GlibMemory_test.cpp 2017-02-23 11:18:54 +0000
528+++ test/gtest/unity/util/GlibMemory/GlibMemory_test.cpp 2017-04-04 09:33:34 +0000
529@@ -89,6 +89,30 @@
530 {
531 return share_glib(g_hash_table_new_full(g_str_hash, g_str_equal, deleteKey, deleteValue));
532 }
533+
534+ static void assignGChar(gchar** in)
535+ {
536+ if (in != nullptr)
537+ {
538+ *in = g_strdup("hi");
539+ }
540+ }
541+
542+ static void assignGError(GError** in)
543+ {
544+ if (in != nullptr)
545+ {
546+ *in = g_error_new_literal(g_key_file_error_quark(), 1, "hello");
547+ }
548+ }
549+
550+ static void assignEmptyGError(GError** in)
551+ {
552+ if (in != nullptr)
553+ {
554+ *in = nullptr;
555+ }
556+ }
557 };
558
559 TEST_F(GlibMemoryTest, Unique)
560@@ -147,4 +171,108 @@
561 }
562 }
563
564+TEST_F(GlibMemoryTest, UPtrAssigner)
565+{
566+ auto gkf = unique_glib(g_key_file_new());
567+
568+ {
569+ GErrorUPtr error;
570+ EXPECT_FALSE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
571+ ASSERT_TRUE(bool(error));
572+ EXPECT_EQ(G_KEY_FILE_ERROR_GROUP_NOT_FOUND, error->code);
573+ }
574+
575+ g_key_file_set_boolean(gkf.get(), "group", "key", TRUE);
576+
577+ {
578+ GErrorUPtr error;
579+ EXPECT_TRUE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
580+ EXPECT_FALSE(bool(error));
581+ }
582+
583+ {
584+ gcharUPtr str;
585+ assignGChar(assign_glib(str));
586+ ASSERT_TRUE(bool(str));
587+ EXPECT_STREQ("hi", str.get());
588+ }
589+
590+ {
591+ GErrorUPtr error;
592+ assignGError(assign_glib(error));
593+ ASSERT_TRUE(bool(error));
594+ assignEmptyGError(assign_glib(error));
595+ ASSERT_FALSE(bool(error));
596+ }
597+}
598+
599+TEST_F(GlibMemoryTest, SPtrAssigner)
600+{
601+ auto gkf = share_glib(g_key_file_new());
602+
603+ {
604+ GErrorSPtr error;
605+ EXPECT_FALSE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
606+ ASSERT_TRUE(bool(error));
607+ EXPECT_EQ(G_KEY_FILE_ERROR_GROUP_NOT_FOUND, error->code);
608+ }
609+
610+ g_key_file_set_boolean(gkf.get(), "group", "key", TRUE);
611+
612+ {
613+ GErrorSPtr error;
614+ EXPECT_TRUE(g_key_file_get_boolean(gkf.get(), "group", "key", assign_glib(error)));
615+ EXPECT_FALSE(bool(error));
616+ }
617+
618+ {
619+ gcharSPtr str;
620+ assignGChar(assign_glib(str));
621+ ASSERT_TRUE(bool(str));
622+ EXPECT_STREQ("hi", str.get());
623+ }
624+
625+ {
626+ GErrorSPtr error;
627+ assignGError(assign_glib(error));
628+ ASSERT_TRUE(bool(error));
629+ assignEmptyGError(assign_glib(error));
630+ ASSERT_FALSE(bool(error));
631+ }
632+}
633+
634+TEST_F(GlibMemoryTest, GCharUPtr)
635+{
636+ {
637+ auto strv = unique_glib(g_strsplit("a b c", " ", 0));
638+ ASSERT_TRUE(bool(strv));
639+ EXPECT_STREQ("a", strv.get()[0]);
640+ EXPECT_STREQ("b", strv.get()[1]);
641+ EXPECT_STREQ("c", strv.get()[2]);
642+ }
643+
644+ {
645+ auto str = unique_glib(g_strdup("hello"));
646+ ASSERT_TRUE(bool(str));
647+ EXPECT_STREQ("hello", str.get());
648+ }
649+}
650+
651+TEST_F(GlibMemoryTest, GCharSPtr)
652+{
653+ {
654+ auto strv = share_glib(g_strsplit("a b c", " ", 0));
655+ ASSERT_TRUE(bool(strv));
656+ EXPECT_STREQ("a", strv.get()[0]);
657+ EXPECT_STREQ("b", strv.get()[1]);
658+ EXPECT_STREQ("c", strv.get()[2]);
659+ }
660+
661+ {
662+ auto str = share_glib(g_strdup("hello"));
663+ ASSERT_TRUE(bool(str));
664+ EXPECT_STREQ("hello", str.get());
665+ }
666+}
667+
668 }

Subscribers

People subscribed via source and target branches

to all changes: