Merge lp:~pete-woods/unity-api/gobject-pointer-functions into lp:unity-api
| Status: | Merged |
|---|---|
| Approved by: | Michi Henning on 2017-01-19 |
| Approved revision: | 265 |
| Merged at revision: | 265 |
| Proposed branch: | lp:~pete-woods/unity-api/gobject-pointer-functions |
| Merge into: | lp:unity-api |
| Diff against target: |
714 lines (+623/-4) 8 files modified
debian/changelog (+7/-0) include/unity/util/GObjectMemory.h (+128/-0) test/gtest/unity/util/CMakeLists.txt (+1/-0) test/gtest/unity/util/GObjectMemory/CMakeLists.txt (+14/-0) test/gtest/unity/util/GObjectMemory/GObjectMemory_test.cpp (+461/-0) test/headers/CMakeLists.txt (+6/-2) test/headers/compile_headers.py (+4/-1) test/headers/includechecker.py (+2/-1) |
| To merge this branch: | bzr merge lp:~pete-woods/unity-api/gobject-pointer-functions |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Michi Henning (community) | 2017-01-17 | Approve on 2017-01-19 | |
| Charles Kerr (community) | 2017-01-12 | Approve on 2017-01-17 | |
| Unity8 CI Bot | continuous-integration | Approve on 2017-01-17 | |
| James Henstridge | 2017-01-18 | Pending | |
|
Review via email:
|
|||
Commit Message
unity::shell::util - add GObject shared pointer utility functions
Description of the Change
In a bunch of different projects, we have C++ code wrapping up GObject-based C APIs (for GDBus, GMenu, etc). When this code is done well, it uses shared_ptr wrappers to manage the lifecycle of the GObjects automatically. However we have a bunch of the same wrappers spread across these codebases. This is an attempt at unifying some of those helpers.
This code is intentionally header-only, so that there can be no excuses about using this functionality in terms of linking to libunity-apiX.so.
| Charles Kerr (charlesk) wrote : | # |
I suggested a couple of improvements inline but basically the code is sound -- the premise, the gobject wrangling, and the tests are all sound.
However, IIUC michi wrote the compile_headers.py test and came up with the policy that it's testing. I'd prefer we ran the exception by him rather than me Approving it without giving him a heads-up.
Refraining from "Approve" for that reason only
| Michi Henning (michihenning) wrote : | # |
I'm fine with the exception to the policy.
I'm wondering though whether the approach Jussi and James came up with quite some time ago wouldn't be preferable:
http://
In particular, the proposal here forces use of a shared_ptr, but that is not always what I want, because the instance may not be meant to be shared. The gobj_ptr approach doesn't have this problem because I can trivially convert it to a shared_ptr if I want sharing.
Would it make sense to either use the already-existing approach or, alternatively, convert the one here to use a unique_ptr?
| Pete Woods (pete-woods) wrote : | # |
Hi guys. Thanks for your input! I've re-done this branch using the code Jussi wrote, with some additions such as adding make_* style helper methods.
I've also imported all the unit tests for this from Jussi, too. I've kept around some of my old tests for the helper functions, though.
| Pete Woods (pete-woods) wrote : | # |
I also added a standard hash function for the gobj_ptr class, so that it behaves inside unordered containers.
The make_gobject function now makes use of std::forward (thanks to michi for showing me the existence of this), so it is quite a bit simpler!
| Unity8 CI Bot (unity8-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:265
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Unity8 CI Bot (unity8-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:266
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Unity8 CI Bot (unity8-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:267
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
| Pete Woods (pete-woods) wrote : | # |
Added a follow-on MR here: https:/
This one adds utility methods for all glib types (a bit more tricky here, as they all have different deleters).
| Pete Woods (pete-woods) wrote : | # |
Thinking about this a bit more. I want to be absolutely sure that using a new class gobj_ptr has a tangible advantage over a unique_ptr with a custom deleter. I'm struggling to find the advantage of gobj_ptr.
The advantage of unique_ptr I can see is that it's automatically convertible to a shared_ptr, whilst preserving the custom deleter. You can also keep the floating ref validation in the factory method.
| Michi Henning (michihenning) wrote : | # |
> Thinking about this a bit more. I want to be absolutely sure that using a new
> class gobj_ptr has a tangible advantage over a unique_ptr with a custom
> deleter. I'm struggling to find the advantage of gobj_ptr.
Using a unique_ptr with a custom deleter would break the unique_ptr contract. That's because, in the standard, the constructor from pointer is noexcept, as are the copy constructor, copy assignment operator, and reset(). In gobj_ptr, they are not noexcept because of the floating reference issue.
(I'm honestly wondering though whether it might not be better to just declare the floating reference issue a precondition and abort if such a pointer is passed. It would avoid all the stuffing around with the not-noexcept methods.)
> The advantage of unique_ptr I can see is that it's automatically convertible
> to a shared_ptr, whilst preserving the custom deleter. You can also keep the
> floating ref validation in the factory method.
Except for the noexcept problem. But it does beg the question... Why not just use this?
struct Deleter
{
void operator()(GObject* p) { g_object_unref(p); }
};
Deleter d;
std::unique_
For the hash function, I don't like the derivation from __hash_base. All that __hash_base provides are the typedefs for result_type and argument_type, which we could define ourselves here. That would avoid relying on a libstdc++ implementation detail that, strictly speaking, we don't even know exists. (Using __hash_base is inherently non-portable and implementation dependent.)
I suspect that you won't like what I'm about to say, but I'll say it anyway...
We have used camel case for type names everywhere so far, and gobj_ptr is what we otherwise use for method names. In the interest of the consistency of the overall API, it would be preferable to rename gobj_ptr to GObjectPtr. That would also be consistent with the quite similar ResourcePtr.
But, before making any more changes, we should figure out why we have gobj_ptr when a unique_ptr with a custom deleter appears to do the same job. Is the floating reference the only reason for having created a separate class? If so, it might be better to just declare the floating reference thing an SEP and use unique_ptr with a custom deleter instead? James, do you remember?
| Pete Woods (pete-woods) wrote : | # |
FYI, the deleter we're using here when it comes to shared_ptr (as opposed to gobj_ptr) is below:
> void g_object_
> g_return_
> g_object_
> }
I would also propose making the methods as follows for the unique_ptr construction:
> template<typename T, typename... Args>
> inline std::unique_ptr<T, > make_gobject(GType object_type,
> const gchar *first_
> return std::unique_ptr<T, decltype(
> (G_TYPE_
> g_object_
> object_type, T), &g_object_deleter);
> }
and:
> template<typename T>
> inline std::unique_ptr<T> unique_gobject(T* ptr) {
> if (ptr != nullptr && g_object_
> g_object_
> throw std::invalid_
> }
> return std::unique_ptr<T, decltype(
> }
This way the factory method can throw the exception safely, also unref the floating object, and we can use unique_ptr with its advantages I mentioned earlier.
| Pete Woods (pete-woods) wrote : | # |
Hmm, LP has munged the code formatting.
| Pete Woods (pete-woods) wrote : | # |
I've updated the branch with what I proposed
| Pete Woods (pete-woods) wrote : | # |
The old stuff is at: lp:~pete-woods/unity-api/gobject-backup
| Pete Woods (pete-woods) wrote : | # |
The more I think about it, the more I don't like the behaviour of the old gobj_ptr class. It was assignable from another gobj_ptr, which is definitely not behaving like a unique_ptr at all. It was behaving like a shared_ptr there.
| Michi Henning (michihenning) wrote : | # |
> The more I think about it, the more I don't like the behaviour of the old
> gobj_ptr class. It was assignable from another gobj_ptr, which is definitely
> not behaving like a unique_ptr at all. It was behaving like a shared_ptr
> there.
I just had a look. It works; after calling release(), you have to explicitly unref the pointer that is returned (but that may not deallocate the instance if the refcount is still non-zero). The reference counts remain sane. It's sort of a hybrid of unique_ptr and shared_ptr, which will be surprising to some people, I guess.
I like your factory/helper method approach. It's clean and re-uses as much from the std library as possible, and there is no need to learn the semantics of a new class.
I left some minor comments inline.
In the doc, you could mention that the template parameter is optional due to template type deduction. For example, this works:
auto u = unique_
auto s = share_gobject(
I would also suggest to add an overload for unique_ptr:
template<typename T, typename D>
inline std::shared_ptr<T>
share_gobject(
{
return std::shared_
}
With this, you can write:
auto u = unique_
auto s = share_gobject(
The shared_ptr constructor remembers the deleter used by the unique_ptr, so GObjectDeleter will still be called.
Anal-retentive comment: the code should use Allman style, not K&R style for the curlies; that's what is used elsewhere in unity-api.
| Pete Woods (pete-woods) wrote : | # |
Thanks for the thorough review and comments! Will get on that now :)
| Pete Woods (pete-woods) wrote : | # |
And nothing wrong with the comment RE braces. I don't know why I got it into my head we were doing K&R here.
| Pete Woods (pete-woods) wrote : | # |
Okay, I've reformatted the code to BSD/Allman and fixed the inline comments. We already discussed the share_gobject override thing in IRC.
- 265. By Pete Woods on 2017-01-19
-
unity::util - add GObject shared memory utility classes and helper methods.
PASSED: Continuous integration, rev:261
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/