Make potentially window destroying functions take simple pointers

Instead of pointer to pointer. The two level of pointers were used to
inform the caller the window has been destroyed (pointer set to NULL).

Now, destruction of windows are now signaled by the return value, which
is marked `must_use`.

Problem with taking pointer to pointer is that they tends to pollute the
function signatures a little bit. And we are not using it well anyways.
A lot of the callers passes pointers to local variables to those
functions, so the information about window destruction doesn't actually
bubble up. So, we switch to use just one level of pointers instead.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

src/compton.c

@@ -11,8 +11,8 @@
 
 #include <X11/Xlib-xcb.h>
 #include <X11/Xlib.h>
-#include <X11/extensions/sync.h>
 #include <X11/Xutil.h>
+#include <X11/extensions/sync.h>
 #include <fcntl.h>
 #include <inttypes.h>
 #include <stdio.h>
@@ -458,10 +458,8 @@ static struct managed_win *paint_preprocess(session_t *ps, bool *fade_running) {
 			add_damage_from_win(ps, w);
 		}
 
-		win_check_fade_finished(ps, &w);
-
-		if (!w) {
-			// the window might have been destroyed because fading finished
+		if (win_check_fade_finished(ps, w)) {
+			// the window has been destroyed because fading finished
 			continue;
 		}
 
@@ -657,10 +655,8 @@ static void rebuild_shadow_exclude_reg(session_t *ps) {
 static void destroy_backend(session_t *ps) {
 	win_stack_foreach_managed_safe(w, &ps->window_stack) {
 		// Wrapping up fading in progress
-		win_skip_fading(ps, &w);
-
-		// `w` might be freed by win_check_fade_finished
-		if (!w) {
+		if (win_skip_fading(ps, w)) {
+			// `w` is freed by win_skip_fading
 			continue;
 		}
 
@@ -1276,7 +1272,7 @@ static void handle_new_windows(session_t *ps) {
 				map_win(ps, mw);
 
 				// This window might be damaged before we called fill_win
-				// and created the damage handle. And there is not way for
+				// and created the damage handle. And there is no way for
 				// us to find out. So just blindly mark it damaged
 				mw->ever_damaged = true;
 				add_damage_from_win(ps, mw);

src/event.c

@@ -263,9 +263,9 @@ static inline void ev_destroy_notify(session_t *ps, xcb_destroy_notify_event_t *
 	auto w = find_win(ps, ev->window);
 	if (w) {
 		if (w->managed) {
-			unmap_win(ps, (struct managed_win **)&w, true);
+			auto _ attr_unused = unmap_win(ps, (struct managed_win *)w, true);
 		} else {
-			destroy_unmanaged_win(ps, &w);
+			destroy_unmanaged_win(ps, w);
 		}
 	}
 }
@@ -280,7 +280,7 @@ static inline void ev_map_notify(session_t *ps, xcb_map_notify_event_t *ev) {
 static inline void ev_unmap_notify(session_t *ps, xcb_unmap_notify_event_t *ev) {
 	auto w = find_managed_win(ps, ev->window);
 	if (w) {
-		unmap_win(ps, &w, false);
+		auto _ attr_unused = unmap_win(ps, w, false);
 	}
 }
 
@@ -306,9 +306,10 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t
 		auto w = find_win(ps, ev->window);
 		if (w) {
 			if (w->managed) {
-				unmap_win(ps, (struct managed_win **)&w, true);
+				auto _ attr_unused =
+				    unmap_win(ps, (struct managed_win *)w, true);
 			} else {
-				destroy_unmanaged_win(ps, &w);
+				destroy_unmanaged_win(ps, w);
 			}
 		}
 

src/win.c

@@ -1584,8 +1584,9 @@ void win_ev_stop(session_t *ps, const struct win *w) {
 	}
 }
 
-static void finish_unmap_win(session_t *ps, struct managed_win **_w) {
-	auto w = *_w;
+/// Finish the unmapping of a window (e.g. after fading has finished).
+/// Doesn't free `w`
+static void finish_unmap_win(session_t *ps, struct managed_win *w) {
 	w->ever_damaged = false;
 	w->reg_ignore_valid = false;
 	w->state = WSTATE_UNMAPPED;
@@ -1600,13 +1601,15 @@ static void finish_unmap_win(session_t *ps, struct managed_win **_w) {
 	w->flags = 0;
 }
 
-static void finish_destroy_win(session_t *ps, struct managed_win **_w) {
-	auto w = *_w;
+/// Finish the destruction of a window (e.g. after fading has finished).
+/// Frees `w`
+static void finish_destroy_win(session_t *ps, struct managed_win *w) {
+	log_trace("Trying to finish destroying (%#010x)", w->base.id);
 
 	if (w->state != WSTATE_UNMAPPED) {
 		// Only UNMAPPED state has window resources freed, otherwise
 		// we need to call finish_unmap_win to free them.
-		finish_unmap_win(ps, _w);
+		finish_unmap_win(ps, w);
 	}
 
 	// Invalidate reg_ignore of windows below this one
@@ -1646,23 +1649,19 @@ static void finish_destroy_win(session_t *ps, struct managed_win **_w) {
 		}
 	}
 	free(w);
-	*_w = NULL;
 }
 
-static void finish_map_win(struct managed_win **_w) {
-	auto w = *_w;
+static void finish_map_win(struct managed_win *w) {
 	w->in_openclose = false;
 	w->state = WSTATE_MAPPED;
 }
 
-void destroy_unmanaged_win(session_t *ps, struct win **_w) {
-	auto w = *_w;
+void destroy_unmanaged_win(session_t *ps, struct win *w) {
 	assert(!w->managed);
 	assert(!w->destroyed);
 	list_remove(&w->stack_neighbour);
 	HASH_DEL(ps->windows, w);
 	free(w);
-	*_w = NULL;
 }
 
 /// Move window `w` so it's before `next` in the list
@@ -1748,13 +1747,13 @@ void restack_top(session_t *ps, struct win *w) {
 	restack_win(ps, w, ps->window_stack.next);
 }
 
-void unmap_win(session_t *ps, struct managed_win **_w, bool destroy) {
-	auto w = *_w;
-
+/// Unmap or destroy a window
+/// @return whether the window is destroyed and freed
+bool unmap_win(session_t *ps, struct managed_win *w, bool destroy) {
 	winstate_t target_state = destroy ? WSTATE_DESTROYING : WSTATE_UNMAPPING;
 
 	if (unlikely(!w)) {
-		return;
+		return false;
 	}
 
 	if (!destroy && w->a._class == XCB_WINDOW_CLASS_INPUT_ONLY) {
@@ -1762,7 +1761,7 @@ void unmap_win(session_t *ps, struct managed_win **_w, bool destroy) {
 		// But we need to remember to destroy them, so future window with
 		// the same id won't be handled incorrectly.
 		// Issue #119 was caused by this.
-		return;
+		return false;
 	}
 
 	log_trace("Unmapping %#010x \"%s\", destroy = %d", w->base.id,
@@ -1776,7 +1775,7 @@ void unmap_win(session_t *ps, struct managed_win **_w, bool destroy) {
 	// If the window is already in the state we want
 	if (unlikely(w->state == target_state)) {
 		log_warn("%s a window twice", destroy ? "Destroying" : "Unmapping");
-		return;
+		return false;
 	}
 
 	if (destroy) {
@@ -1792,11 +1791,11 @@ void unmap_win(session_t *ps, struct managed_win **_w, bool destroy) {
 		if (unlikely(!destroy)) {
 			log_warn("Unmapping an already unmapped window %#010x %s twice",
 			         w->base.id, w->name);
-			return;
+			return false;
 		}
 		// Window is already unmapped, or is an Input Only window, just destroy it
-		finish_destroy_win(ps, _w);
-		return;
+		finish_destroy_win(ps, w);
+		return true;
 	}
 
 	// Note we don't update focused window here. This will either be
@@ -1825,42 +1824,47 @@ void unmap_win(session_t *ps, struct managed_win **_w, bool destroy) {
 #endif
 
 	if (!ps->redirected) {
-		win_skip_fading(ps, _w);
+		return win_skip_fading(ps, w);
 	}
+	return false;
 }
 
 /**
  * Execute fade callback of a window if fading finished.
+ *
+ * @return whether the window is destroyed and freed
  */
-void win_check_fade_finished(session_t *ps, struct managed_win **_w) {
-	auto w = *_w;
+bool win_check_fade_finished(session_t *ps, struct managed_win *w) {
 	if (w->state == WSTATE_MAPPED || w->state == WSTATE_UNMAPPED) {
 		// No fading in progress
 		assert(w->opacity_target == w->opacity);
-		return;
+		return false;
 	}
 	if (w->opacity == w->opacity_target) {
 		switch (w->state) {
-		case WSTATE_UNMAPPING: return finish_unmap_win(ps, _w);
-		case WSTATE_DESTROYING: return finish_destroy_win(ps, _w);
-		case WSTATE_MAPPING: return finish_map_win(_w);
+		case WSTATE_UNMAPPING: finish_unmap_win(ps, w); return false;
+		case WSTATE_DESTROYING: finish_destroy_win(ps, w); return true;
+		case WSTATE_MAPPING: finish_map_win(w); return false;
 		case WSTATE_FADING: w->state = WSTATE_MAPPED; break;
 		default: unreachable;
 		}
 	}
+
+	return false;
 }
 
 /// Skip the current in progress fading of window,
 /// transition the window straight to its end state
-void win_skip_fading(session_t *ps, struct managed_win **_w) {
-	auto w = *_w;
+///
+/// @return whether the window is destroyed and freed
+bool win_skip_fading(session_t *ps, struct managed_win *w) {
 	if (w->state == WSTATE_MAPPED || w->state == WSTATE_UNMAPPED) {
 		assert(w->opacity_target == w->opacity);
-		return;
+		return false;
 	}
 	log_trace("Skipping fading process of window %#010x (%s)", w->base.id, w->name);
 	w->opacity = w->opacity_target;
-	win_check_fade_finished(ps, _w);
+	return win_check_fade_finished(ps, w);
 }
 
 /**
@@ -1903,7 +1907,7 @@ void map_win(session_t *ps, struct managed_win *w) {
 	}
 
 	if (w->state == WSTATE_UNMAPPING) {
-		win_skip_fading(ps, &w);
+		CHECK(!win_skip_fading(ps, w));
 		// We skipped the unmapping process, the window was rendered, now it is
 		// not anymore. So we need to mark then unmapping window as damaged.
 		add_damage_from_win(ps, w);
@@ -2018,7 +2022,7 @@ void map_win(session_t *ps, struct managed_win *w) {
 #endif
 
 	if (!ps->redirected) {
-		win_skip_fading(ps, &w);
+		CHECK(!win_skip_fading(ps, w));
 		assert(w);
 	}
 }

src/win.h

@@ -356,9 +356,10 @@ void restack_bottom(session_t *ps, struct win *w);
 /// Move window `w` to the top of the stack
 void restack_top(session_t *ps, struct win *w);
 /// Unmap or destroy a window
-void unmap_win(session_t *ps, struct managed_win **, bool destroy);
-/// Destroy an unmanaged window
-void destroy_unmanaged_win(session_t *ps, struct win **w);
+/// @return whether the window is destroyed and freed
+bool must_use unmap_win(session_t *ps, struct managed_win *, bool destroy);
+/// Destroy and free an unmanaged window
+void destroy_unmanaged_win(session_t *ps, struct win *w);
 
 void map_win(session_t *ps, struct managed_win *w);
 void map_win_by_id(session_t *ps, xcb_window_t id);
@@ -366,14 +367,16 @@ void map_win_by_id(session_t *ps, xcb_window_t id);
 /**
  * Execute fade callback of a window if fading finished.
  */
-void win_check_fade_finished(session_t *ps, struct managed_win **_w);
+bool must_use win_check_fade_finished(session_t *ps, struct managed_win *w);
 
 // Stop receiving events (except ConfigureNotify, XXX why?) from a window
 void win_ev_stop(session_t *ps, const struct win *w);
 
 /// Skip the current in progress fading of window,
 /// transition the window straight to its end state
-void win_skip_fading(session_t *ps, struct managed_win **_w);
+///
+/// @return whether the window is destroyed and freed
+bool must_use win_skip_fading(session_t *ps, struct managed_win *w);
 /**
  * Find a managed window from window id in window linked list of the session.
  */