shared: add nm_clear_pointer() and implement existing nm_clear_*() based on it

Add an alternative to g_clear_pointer(). The differences are:

  - nm_clear_pointer() is more type safe as it does not cast neither the
    pointer nor the destroy function. Commonly, the types should be compatible
    and not requiring a cast. Casting in the macro eliminates some of the
    compilers type checking. For example, while
       g_clear_pointer (&priv->hash_table, g_ptr_array_unref);
    compiles, nm_clear_pointer() would prevent such an invalid use.

  - also, clear the destination pointer *before* invoking the destroy
    function. Destroy might emit signals (like weak-pointer callbacks
    of GArray clear functions). Clear the destination first, so that
    we don't leave a dangling pointer there.

  - return TRUE/FALSE depending on whether there was a pointer to clear.

I tested that redefining g_clear_pointer()/g_clear_object() with our
more typesafe nm_* variants still compiles and indicates no bugs. So
that is good. It's not really expected that turning on more static checks
would yield a large number of bugs, because generally our code is in a good
shape already. We have few such bugs, because we already turn all all warnings
and extra checks that make sense. That however is not an argument for
not introducing (and using) a more resticted implementation.

1 files changed, 28 insertions, 27 deletions

diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h
index 62d4f17bc9..f0d7374eb4 100644
--- a/shared/nm-utils/nm-macros-internal.h
+++ b/shared/nm-utils/nm-macros-internal.h
@@ -793,6 +793,32 @@ nm_g_object_unref (gpointer obj)
 		_changed; \
 	})
 
+#define nm_clear_pointer(pp, destroy) \
+	({ \
+		typeof (*(pp)) *_pp = (pp); \
+		typeof (*_pp) _p; \
+		gboolean _changed = FALSE; \
+		\
+		if (   _pp \
+		    && (_p = *_pp)) { \
+			_nm_unused gconstpointer _p_check_is_pointer = _p; \
+			\
+			*_pp = NULL; \
+			/* g_clear_pointer() assigns @destroy first to a local variable, so that
+			 * you can call "g_clear_pointer (pp, (GDestroyNotify) destroy);" without
+			 * gcc emitting a warning. We don't do that, hence, you cannot cast
+			 * "destroy" first.
+			 *
+			 * On the upside: you are not supposed to cast fcn, because the pointer
+			 * types are preserved. If you really need a cast, you should cast @pp.
+			 * But that is hardly ever necessary. */ \
+			(destroy) (_p); \
+			\
+			_changed = TRUE; \
+		} \
+		_changed; \
+	})
+
 /* basically, replaces
  *   g_clear_pointer (&location, g_free)
  * with
@@ -803,35 +829,10 @@ nm_g_object_unref (gpointer obj)
  * pointer or points to a const-pointer.
  */
 #define nm_clear_g_free(pp) \
-	({  \
-		typeof (*(pp)) *_pp = (pp); \
-		typeof (**_pp) *_p; \
-		gboolean _changed = FALSE; \
-		\
-		if (  _pp \
-		    && (_p = *_pp)) { \
-			*_pp = NULL; \
-			g_free (_p); \
-			_changed = TRUE; \
-		} \
-		_changed; \
-	})
+	nm_clear_pointer (pp, g_free)
 
 #define nm_clear_g_object(pp) \
-	({ \
-		typeof (*(pp)) *_pp = (pp); \
-		typeof (**_pp) *_p; \
-		gboolean _changed = FALSE; \
-		\
-		if (   _pp \
-		    && (_p = *_pp)) { \
-			nm_assert (G_IS_OBJECT (_p)); \
-			*_pp = NULL; \
-			g_object_unref (_p); \
-			_changed = TRUE; \
-		} \
-		_changed; \
-	})
+	nm_clear_pointer (pp, g_object_unref)
 
 static inline gboolean
 nm_clear_g_source (guint *id)