Bug 101895 - Add dbus_clear_connection(), etc. macros
Summary: Add dbus_clear_connection(), etc. macros
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/commits/...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks: 101354
  Show dependency treegraph
 
Reported: 2017-07-24 12:31 UTC by Simon McVittie
Modified: 2017-07-30 11:41 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Implement dbus_clear_connection(), etc. (8.26 KB, patch)
2017-07-24 12:33 UTC, Simon McVittie
Details | Splinter Review
tests: Use dbus_clear_connection etc. in a couple of tests (15.56 KB, patch)
2017-07-24 12:33 UTC, Simon McVittie
Details | Splinter Review
Implement dbus_clear_connection(), etc. (8.78 KB, patch)
2017-07-28 17:51 UTC, Simon McVittie
Details | Splinter Review
tests: Use dbus_clear_connection etc. in a couple of tests (15.56 KB, patch)
2017-07-28 17:52 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2017-07-24 12:31:27 UTC
GLib has g_clear_object() and g_clear_pointer() macros which are really useful:

    GProxyResolver *object = NULL;    /* or any other GObject */
    struct { ...; GArray *arr; ...; } *my_struct;

    ...

    if (...)
      {
        object = ...;
        my_struct->arr = ...;
      }

  out:
    g_clear_object (&object);
    g_clear_pointer (&my_struct->arr, g_array_unref);

They were inspired by Python's Py_CLEAR_OBJECT() which has the same semantics. They expand to something like this:

    GArray *tmp = *(&arr);    /* or equivalently, arr */

    *(&arr) = NULL;

    if (tmp != NULL)
      g_array_unref (tmp);

(In particular the use of a temporary variable means the struct member is cleared before the user-supplied free-function is called, which is safer if the free-function might cause reentrancy.)

We can make libdbus less painful to program in by doing something similar.
Comment 1 Simon McVittie 2017-07-24 12:33:28 UTC
Created attachment 132864 [details] [review]
Implement dbus_clear_connection(), etc.

These are inspired by GLib's g_clear_pointer() and g_clear_object(),
which in turn is descended from CPython's Py_CLEAR_OBJECT. They should
make our code a lot less repetitive.
Comment 2 Simon McVittie 2017-07-24 12:33:51 UTC
Created attachment 132865 [details] [review]
tests: Use dbus_clear_connection etc. in a couple of  tests

This is just enough to demonstrate that they work - I'm deliberately
not doing a mass change throughout all tests, and we should definitely
not rush to introduce these into production code, because it would
hinder cherry-picking and merging fixes between branches. However,
new code on master can use them freely.
Comment 3 Simon McVittie 2017-07-24 12:35:54 UTC
These are deliberately not called things like dbus_message_clear(), for two reasons:

* You might think dbus_message_clear() does something else, like maybe
  removing the body and all the headers

* They are not really pseudo-OO instance methods on a DBusMessage etc. -
  their first argument is a DBusMessage **, not a DBusMessage * as you
  would expect for a pseudo-OO method
Comment 4 Philip Withnall 2017-07-28 10:16:16 UTC
Comment on attachment 132864 [details] [review]
Implement dbus_clear_connection(), etc.

Review of attachment 132864 [details] [review]:
-----------------------------------------------------------------

YES. r+++++
Comment 5 Philip Withnall 2017-07-28 10:18:18 UTC
Comment on attachment 132865 [details] [review]
tests: Use dbus_clear_connection etc. in a couple of  tests

Review of attachment 132865 [details] [review]:
-----------------------------------------------------------------

r+
Comment 6 Simon McVittie 2017-07-28 17:51:54 UTC
Created attachment 133104 [details] [review]
Implement dbus_clear_connection(), etc.

These are inspired by GLib's g_clear_pointer() and g_clear_object(),
which in turn is descended from CPython's Py_CLEAR_OBJECT. They should
make our code a lot less repetitive.

---

Be more careful with types and casts, to keep C++ compilers happy
(see comment). This broke the build with mingw on Travis-CI, because
we use a tiny amount of C++ on Windows to get code run at library init
time in a non-horrific way.
Comment 7 Simon McVittie 2017-07-28 17:52:28 UTC
Created attachment 133105 [details] [review]
tests: Use dbus_clear_connection etc. in a couple of  tests

This is just enough to demonstrate that they work - I'm deliberately
not doing a mass change throughout all tests, and we should definitely
not rush to introduce these into production code, because it would
hinder cherry-picking and merging fixes between branches. However,
new code on master can use them freely.

---

I don't think I changed this, but I've lost track.
Comment 8 Philip Withnall 2017-07-29 12:45:45 UTC
Comment on attachment 133104 [details] [review]
Implement dbus_clear_connection(), etc.

Review of attachment 133104 [details] [review]:
-----------------------------------------------------------------

r+ still
Comment 9 Simon McVittie 2017-07-30 11:41:33 UTC
Thanks, merged for 1.11.18.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.