dispose() would previously set the "handlers" pointer to NULL. But
dispose() also calls g_signal_group_gc_handlers(), which requires this
pointer to be not NULL.
This means, dispose() could not be called multiple times. Which is a
good practice to allow, because g_object_run_dispose() and object
resurrection both requires that dispose() can be called more than once
per object.
Fix that problem, by leaving the array until finalize().
Fixes: dd43471f60 ('gobject: add GSignalGroup')
We call g_object_weak_release_all() at two places.
Once right before finalize(). At this point, the object is definitely
going to be destroyed, and the user must no longer resurrect it or
subscribe new weak notifications. In that case, we really want to
notify/release all weak notifications.
However, we also call it from g_object_real_dispose(). During dispose,
the API allows the user to resurrect an object. Granted, that is
probably not something anybody should do, but GObject makes a reasonable
attempt to support that.
A possible place to resurrect (and subscribe new weak notifications) is
when GObject calls g_object_real_dispose().
static void
g_object_real_dispose (GObject *object)
{
g_signal_handlers_destroy (object);
/* GWeakNotify and GClosure can call into user code */
g_object_weak_release_all (object);
closure_array_destroy_all (object);
}
But previously, g_object_weak_release_all() would continue iterating
until there are no more weak notifications left. So while the user can
take a strong reference and resurrect the object, their attempts to
register new weak notifications are thwarted.
Instead, when the loop in g_object_weak_release_all() starts, remember
the initial number of weak notifications, and don't release more than
that. Note that WeakRefStack preserves the order of entries, so by
maintaining the "remaining_to_notify" counter we know when to stop.
Note that this brings also an earlier behavior back, where we would call
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
This would take out the entire WeakRefStack at once and notify the weak
notifications registered at the time. But subsequent registrations would
not be released/notified yet.
It seems bad style, to use a naive realloc() +1 increment each time a new
element gets added. Instead, remember the allocation size and double the
buffer size on buffer grow. This way we get linear amortized runtime
complexity for buffer growth.
Well, WeakRefStack uses a flat array for tracking the entires. We anyway
need to search and memmove() the entries and are thus O(n) anyway. We do
that, because it allows for relatively simple code while being memory
efficient. Also, we do expect only a reasonably small number of weak
notifications in the first place.
I still think it makes sense to avoid the O(n) number of realloc() calls
on top of that. Note that we do this while holding the (per-object)
lock. It's one thing to do a linear search or a memmove(). It's another
to do a (more expensive) realloc().
Also, shrink the buffer during g_object_weak_unref() to get rid of
excess memory.
Also, note that the initial allocation only allocates space for the
first item. I think that makes sense, because I expect that many objects
will only get a single weak notification registered. So this allocation
should not yet have excess memory allocated.
Also, note that the "flexible" array member WeakRefStack.weak_refs has a
length of 1. Maybe we should use C99 flexible array members ([]) or the
pre-C99 workaround ([0]). Anyway. Previously, we would always allocate
space for that extra one tuple, but never use it. Fix that too.
Previously, at two places (in g_object_real_dispose() and shortly before
finalize()), we would call
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
This clears @quark_weak_notifies at once and then invokes all
notifications.
This means, if you were inside a notification callback and called
g_object_weak_unref() on the object for *another* weak-reference, then
an exception failed:
GLib-GObject-FATAL-CRITICAL: g_object_weak_unref_cb: couldn't find weak ref 0x401320(0x16b9fe0)
Granted, maybe inside a GWeakNotify you shouldn't call much of anything
on where_the_object_was. However, unregistering things (like calling
g_object_weak_unref()) should still reasonably work.
Instead, now remove each weak notification one by one and invoke it.
As we now invoke the callbacks in a loop, if a callee registers a new
callback, then that one gets unregistered right away too. Previously,
we would during g_object_real_dispose() only notify the notifications
that were present when the loop starts. This is similar to what happens
in closure_array_destroy_all(). This is a change in behavior, but it
will be fixed in a separate follow-up commit.
https://gitlab.gnome.org/GNOME/glib/-/issues/1002
g_object_weak_unref() would have done a fast-removal of the entry, which
messes up the order of the weak notifications.
During destruction of the object we emit the weak notifications. They
are emitted in the order in which they were registered (FIFO). Except,
when a g_object_weak_unref() messes up the order. Avoid that and
preserve the order.
Now, do a memmove(), which is O(n). But note that we already track weak
references in a flat array that requires a O(n) linear search. Thus,
g_object_weak_unref() was already O(n) and that didn't change. More
importantly, users are well advised to limit themselves to a reasonably
small number of weak notifications. And for small n, the linear search
and the memmove() is an efficient solution.
From `man 3 ctime`:
According to POSIX.1, localtime() is required to behave as though tzset(3)
was called, while localtime_r() does not have this requirement.
For portable code, tzset(3) should be called before localtime_r().
At the time when this code was added ([1]), the code and the comment was
correct. g_object_run_dispose() did not clear GWeakRef.
That was later adjusted to clear them ([2]), but at various times it was
not ensured that the GWeakRef was cleared *before* the weak notification
is emitted.
This is now fixed, and the checks for "where_the_object_was" are no
longer necessary. Drop them.
I considered to keep the checks just to be extra safe. But we need to
rely on how g_object_run_dispose() works in detail. By now there is a
test that checks GWeakRef are cleared before emitting the notifications,
so we should not accidentally mess this up and the code is no longer
needed.
[1] commit e82eb490fe ('Handle the case of g_object_run_dispose() in GBinding')
[2] commit a7262d6357 ('gobject: Cleanup weak locations data as part of dispose')
This changes behavior from commit [1] most similar to what was before.
The point of g_object_run_dispose() is to break reference cycles to
bring down an object. We don't expect the object to take new references
to keep it alive for longer. We probably also don't expect it to
register new weak references. We also don't expect the dispose() callees
to check g_weak_ref_get() for the object. In that case, this change
makes not difference.
Note that during g_object_run_dispose() the ref count does not yet go to
zero, still we clear GWeakRef. As such, GWeakRef rather tracks when
objects get disposed, instead of when the ref count really goes to zero.
That is intentional (e.g. issue [2]).
But compare to g_object_unref(), where we also clear GWeakRef *before*
calling dispose. That makes more sense, because inside dispose() (and
for example during weak notifications), we probably want to see that
g_weak_ref_get() indicates the object is already disposed. For that
reason, it seems more correct to clear out the GWeakRef before calling
dispose().
Also, the dispose() callees (e.g. the weak notifications) might refuse to
let the object die by intentionally keeping strong references around.
Not sure why they would do that, it is similar to resurrecting an object
during dispose(). But if they do, they might also want to register new
GWeakRef. In that case, we wouldn't want to unset those newly set
GWeakRef unconditionally right after.
In most cases, it shouldn't make a difference. In the case where it
does, this is the more sensible order of doing things.
[1] commit 2952cfd7a7 ('gobject: drop clearing quark_weak_locations from g_object_real_dispose()')
[2] https://gitlab.gnome.org/GNOME/glib/-/issues/2266
During object initialization, we may want to freeze the notifications,
but only do so once (and once unfreeze at the end).
Rework how that was done. We can avoid an additional GData lookup.
By now, GObjectNotifyQueue gets reallocated. So quite possibly if we
keep the queue, it is a dangling pointer.
That is error prone, but it's also unnecessary. All we need to know is
whether we bumped the freeze count and need to unfreeze. The queue
itself was not useful, because we anyway must take a lock (via
g_datalist_id_update_atomic()) to do anything with it.
Instead, use a nqueue_is_frozen boolean variable.
GSList is almost in all use cases a bad choice. It's bad for locality
and requires a heap allocation per entry.
Instead, use an array, and grow the buffer exponentially via realloc().
Now, that we use g_datalist_id_update_atomic(), it is also easy to
update the pointer. Hence, the GObjectNotifyQueue struct does not point
to an array of pspecs. Instead the entire GObjectNotifyQueue itself gets
reallocated, thus saving one heap allocation for the separate head
structure.
The members of `struct segment_command` appear to have type `uint32_t`,
so definitely need casting to the machine’s integer pointer type before
doing pointer arithmetic on them.
See https://developer.apple.com/documentation/kernel/segment_command
Tested only on macOS CI as I don’t have access to a macOS machine.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Because of the generic nature of `GError`, `g_set_error()` has to take
an `int`, but `g_file_error_from_errno()` returns a `GFileError`. The
macOS CI runner decides that’s a good reason to emit
`-Wsign-conversion`.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
This struct is only ever heap allocated, and enums are always the same
size as an int (or unsigned int), so it won’t change size.
The struct doesn’t correspond to any mmapped structure from a
typelib file.
This should fix some `-Wsign-conversion` warnings (curiously only seen
on the macOS CI runner) by using the most specific type.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Parsing scripts using g_shell_parse_argv() line by line, it's useful to
have empty comments, so one can write pragraphs etc, e.g.
# This is a comment with multiple paragraphs.
#
# It's useful to split things up like this at times.
#
# The empty comment characters makes it clear the paragraphs form a
# single comment.
If g_file_set_contents{_full,} is replacing an existing file, require
that the tmpfile have the same mode as the existing file.
This prevents the umask from taking effect for consistent writes to
existing files.
ClosesGNOME/dconf#76
We can tighten up the types which are being used, to prevent the
warnings. Not everything in the world has to be a `guint`.
These warnings only showed up on the macOS CI runner.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
These only show up on macOS. Apparently it’s more sensitive to assigning
`gboolean` (which is secretly `int`) to a `guint` bitfield. 🤷
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
These don’t show up for me on Linux, but are now causing CI failures on
macOS (https://gitlab.gnome.org/GNOME/glib/-/jobs/5006543):
```
../gobject/gclosure.c:923:40: error: implicit conversion changes signedness: 'gboolean' (aka 'int') to 'guint' (aka 'unsigned int') [-Werror,-Wsign-conversion]
ATOMIC_SET (closure, in_marshal, in_marshal);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This makes the code a little clearer. In most cases, it’s not a
functional change.
In a few cases, the values are different. I believe the original values
were incorrect (accidentally transposed, perhaps). This never caused an
issue because they were all immediately overwritten during construction
of a `GParamSpec`: these values were defaults in the `instance_init`
vfunc of the `GTypeInstance` for a `GParamSpec`, but the actual min/max
for the `GParamSpec` instance were immediately written over them in the
constructor (such as `g_param_spec_int()`).
Spotted in !4593.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Notably without the (array), the argument of g_byte_array_append appears
as
<type name="guint8" c:type="const guint8*"/>
which will cause the bindings to pass a byte, not a pointer.
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
