Created attachment 263658 [details] [review]
glib: Add missing (allow-none) annotations

Add various (allow-none) annotations which were missing from a variety
of functions. This includes adding some stub documentation comments for
the assertion macro error functions, which weren’t previously
documented. The new comments are purely to allow for annotations.

Review of attachment 263658 [details] [review]:

::: glib/gerror.c
@@ +602,3 @@
 /**
  * g_propagate_error:
+ * @dest: (allow-none): error return location

GError** can always implicitly be NULL, but this is a slightly atypical use of that, I suppose, so maybe this does indeed make sense to explicitly note.

::: glib/gmessages.c
@@ +1088,3 @@
+ * @pretty_function:
+ * @expression: (allow-none):
+ */

not sure it makes sense to provide annotations for functions that are not part of our public API...

::: glib/gshell.c
@@ +612,3 @@
  * g_shell_parse_argv:
  * @command_line: command line to parse
+ * @argcp: (out) (allow-none): return location for number of args

this type of change is in a few places... pretty sure the convention is that all (out) parameters should allow NULL to be passed to ignore the result.  (allow-none) in this context would mean that the result itself, as returned via that parameter, could be NULL (which doesn't make sense since this is an int...)

would appreciate confirmation from Colin.

(In reply to comment #2)
> 
> this type of change is in a few places... pretty sure the convention is that
> all (out) parameters should allow NULL to be passed to ignore the result. 
> (allow-none) in this context would mean that the result itself, as returned via
> that parameter, could be NULL (which doesn't make sense since this is an
> int...)

Unfortunately, not all APIs allow passing NULL for out parameters.  I forget which they are, but they exist.  The present state is that (allow-none) is needed mainly for Vala I think - both gjs and pygobject will *always* retrieve out parameters, since the languages have no well-defined way to say "please ignore this output value for invocation".

So I think whether the *result* can be null would have to be a new annotation like (nullable).

That's my understanding of the current state.

(nullable) is discussed in bug 660879

(In reply to comment #2)
> ::: glib/gerror.c
> @@ +602,3 @@
>  /**
>   * g_propagate_error:
> + * @dest: (allow-none): error return location
> 
> GError** can always implicitly be NULL, but this is a slightly atypical use of
> that, I suppose, so maybe this does indeed make sense to explicitly note.

Yup.

> ::: glib/gmessages.c
> @@ +1088,3 @@
> + * @pretty_function:
> + * @expression: (allow-none):
> + */
> 
> not sure it makes sense to provide annotations for functions that are not part
> of our public API...

You're going to hate me for saying this, but it's needed to silence some static analysis warnings. I would argue that those functions are semi-public, since they're exported from the library and explicitly used in macros. Since GIR annotations are arguably part of the symbol information and not the documentation, they should be present for all exported symbols.

Created attachment 275624 [details] [review]
glib: Add missing (allow-none) annotations

Add various (allow-none) annotations which were missing from a variety
of functions. This includes adding some stub documentation comments for
the assertion macro error functions, which weren’t previously
documented. The new comments are purely to allow for annotations, and
hence are marked as (skip) to prevent the symbols appearing in the GIR
file.

This version is rebased on today’s master and has (skip) annotations added to the internal APIs.

Review of attachment 275624 [details] [review]:

::: glib/gatomic.c
@@ +308,3 @@
  * memory barrier (before the get).
  *
+ * Returns: (allow-none): the value of the pointer

No.  We do not have (allow-none) on return values.  This is what the coming support for (nullable) is.

::: glib/gconvert.c
@@ +873,3 @@
  *                 bytes to occur inside strings. In that case, using -1
  *                 for the @len parameter is unsafe)
+ * @bytes_read: (out) (allow-none): location to store the number of bytes in the

Same here.  This will be (optional).

Seeing all of these patches, I almost wonder if we always want to put (optional) (out) since this is likely to be the "normal case"... Unfortunately, we have allow-none in many of these cases already anyway...

::: glib/gmessages.c
@@ +1076,3 @@
 
+/**
+ * g_return_if_fail_warning: (skip)

These are fine if they help the analyzer...

Briefly discussed this morning at the hackfest, and we’re going to hold this patch until bug #660879 is committed.

Created attachment 275881 [details] [review]
glib: Add missing (nullable) and (optional) annotations

Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.

This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.

Updated patch which uses (nullable) and (optional) from bug #660879. This can’t be merged until the patches from bug #660879 are merged.

Created attachment 275958 [details] [review]
glib: Add missing (nullable) and (optional) annotations

Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.

This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.

Created attachment 275965 [details] [review]
glib: Add missing (nullable) and (optional) annotations

Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.

This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.

Review of attachment 275965 [details] [review]:

Most things look good here, but there are still a few unresolved conceptual issues to argue about.  See below.

::: glib/gatomic.c
@@ +308,3 @@
  * memory barrier (before the get).
  *
+ * Returns: (nullable): the value of the pointer

These additions are suspect.

One suspects that void* may always have the possibility of being NULL, although I understand why a static analysis tool written for C may not see it that way...

On the other hand, we often use gpointer as a return value for GObject in place that we very much expect it not to be NULL (like g_object_ref()).  Do you think that it might make sense to try and treat gpointer as always-null but then allow annotating the return type explicitly as a GObject for the other sorts of cases (and possibly adding (nullable) for those, as appropriate)?

::: glib/gerror.c
@@ +499,3 @@
 /**
  * g_error_matches:
+ * @error: (nullable): a #GError or %NULL

I think we agreed that we should remove the "or %NULL" language now that we have (nullable)

@@ +532,3 @@
 /**
  * g_set_error:
+ * @err: (out callee-allocates) (optional): a return location for a #GError,

Isn't callee-allocation the default?

@@ +608,3 @@
  * The error variable @dest points to must be %NULL.
+ *
+ * @src must be non-%NULL.

Please not these sorts of additions -- the new rule is "non-%NULL unless specified (nullable)".

::: glib/ghash.c
@@ +141,3 @@
  * GHFunc:
  * @key: a key
+ * @value: (nullable): the value corresponding to the key

... and here comes my fear of (nullable) gpointer annotations everywhere...

::: glib/gstrfuncs.c
@@ +337,3 @@
 /**
  * g_strdup:
+ * @str: (nullable): the string to duplicate

I guess the return value here should grow 'nullable' as well, right?

@@ +595,3 @@
  * g_strtod:
  * @nptr:    the string to convert to a numeric value.
+ * @endptr: (out) (transfer none) (optional): if non-%NULL, it returns the

These sorts of parameters are extremely problematic from a bindings standpoint.

I guess this helps the static analysis stuff, though?

::: glib/gutf8.c
@@ +437,3 @@
  * g_unichar_to_utf8:
  * @c: a Unicode character code
+ * @outbuf: (out caller-allocates) (optional): output buffer, must have at

I'd like someone who has a better idea about introspection to look over some of these changes...  I feel like we may start to break our introspection API in some of these cases.

(In reply to comment #14)
> One suspects that void* may always have the possibility of being NULL

Yeah, that makes sense.

> On the other hand, we often use gpointer as a return value for GObject in place
> that we very much expect it not to be NULL (like g_object_ref()).

In cases where a gpointer is only being used for C convenience, it should be annotated, eg, "(type GObject)".

(In reply to comment #14)
> ::: glib/gatomic.c
> @@ +308,3 @@
>   * memory barrier (before the get).
>   *
> + * Returns: (nullable): the value of the pointer
> 
> These additions are suspect.
> 
> One suspects that void* may always have the possibility of being NULL, although
> I understand why a static analysis tool written for C may not see it that
> way...
>
> On the other hand, we often use gpointer as a return value for GObject in place
> that we very much expect it not to be NULL (like g_object_ref()).  Do you think
> that it might make sense to try and treat gpointer as always-null but then
> allow annotating the return type explicitly as a GObject for the other sorts of
> cases (and possibly adding (nullable) for those, as appropriate)?

I tried that, and it produces too many awkward corner cases where we legitimately return a non-nullable gpointer, but have no type we can annotate it with. For example, g_malloc(). It’s important that functions like this are correctly annotated or the static analysis loses a lot of its power.

I suggest we go with a more restricted modification: automatically marking all (closure) parameters as nullable, and manually marking other corner cases (like the g_atomic_pointer_get()) as (nullable). I suspect we won’t need too many of the latter, though we will need to add a lot of missing (closure) annotations.
 
> ::: glib/gerror.c
> @@ +499,3 @@
>  /**
>   * g_error_matches:
> + * @error: (nullable): a #GError or %NULL
> 
> I think we agreed that we should remove the "or %NULL" language now that we
> have (nullable)

Done.

> @@ +532,3 @@
>  /**
>   * g_set_error:
> + * @err: (out callee-allocates) (optional): a return location for a #GError,
> 
> Isn't callee-allocation the default?

It might be, but I think it’s better to be explicit if we’re adding the annotation anyway.

> @@ +608,3 @@
>   * The error variable @dest points to must be %NULL.
> + *
> + * @src must be non-%NULL.
> 
> Please not these sorts of additions -- the new rule is "non-%NULL unless
> specified (nullable)".

I was thinking about that when I wrote this, but I think g_propagate_error() is a sufficiently special case that this should be made explicit. Especially since GError* is normally NULL.

> ::: glib/gstrfuncs.c
> @@ +337,3 @@
>  /**
>   * g_strdup:
> + * @str: (nullable): the string to duplicate
> 
> I guess the return value here should grow 'nullable' as well, right?

Yup, added.

> @@ +595,3 @@
>   * g_strtod:
>   * @nptr:    the string to convert to a numeric value.
> + * @endptr: (out) (transfer none) (optional): if non-%NULL, it returns the
> 
> These sorts of parameters are extremely problematic from a bindings standpoint.
> 
> I guess this helps the static analysis stuff, though?

It does. I guess for static analysis, you should start thinking of C as a binding now too. So the annotations have to be correct for all functions (eventually) — even if those functions are completely useless to bindings.

> ::: glib/gutf8.c
> @@ +437,3 @@
>   * g_unichar_to_utf8:
>   * @c: a Unicode character code
> + * @outbuf: (out caller-allocates) (optional): output buffer, must have at
> 
> I'd like someone who has a better idea about introspection to look over some of
> these changes...  I feel like we may start to break our introspection API in
> some of these cases.

Yes, that would be useful.

Created attachment 276003 [details] [review]
glib: Add missing (nullable) and (optional) annotations

Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.

This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.

(In reply to comment #16)
> I suggest we go with a more restricted modification: automatically marking all
> (closure) parameters as nullable, and manually marking other corner cases (like
> the g_atomic_pointer_get()) as (nullable). I suspect we won’t need too many of
> the latter, though we will need to add a lot of missing (closure) annotations.

Filed (with patches) as bug #729660.

Ryan, ping?

Review of attachment 276003 [details] [review]:

I'm still unhappy to see (nullable) all over the generic pointer-type arguments and return values for things like atomics and hashtables...

I'd almost prefer if we invented some new way of marking the g_malloc() sort of cases with a "this never returns NULL".

We're now getting quite far along the path where we stop using the existing annotations as a way to get more information for static analysis to the point where we introduce new types of tags specifically for analysis... That's OK, but I'd really like some more feedback from introspection/docs people about it first.

(In reply to comment #20)
> Review of attachment 276003 [details] [review]:
> 
> I'm still unhappy to see (nullable) all over the generic pointer-type arguments
> and return values for things like atomics and hashtables...
> 
> I'd almost prefer if we invented some new way of marking the g_malloc() sort of
> cases with a "this never returns NULL".

So we could change the default assumptions from:
 • gpointer [in|out|inout] values must not be NULL unless there's a (nullable) annotation
to:
 • gpointer in arguments must not be NULL unless there's a (nullable) annotation
 • gpointer out arguments may be NULL unless there's a (non-nullable) annotation (*changed*)
 • gpointer inout arguments must not be NULL unless there's a (nullable) annotation

Here, 'out' arguments include return values, and I'm talking about nullability of the returned value, not of the out pointer (i.e. not talking about (optional)).

This should avoid being unsafe for bindings in the sense that bindings may allow passing NULL to a function which will crash on it. However, making this change and not adding all the appropriate (non-nullable) annotations may mean regressions in binding functionality where they won't allow passing NULL into a function where they would before.

I was thinking more like:

 - a type of gpointer is always assumed to accept and return NULL unless either:

   - annotated with (type GObject) and not also with (nullable)

  or

   - explicitly annotated (never-null)



So we would annotate the return value of g_malloc() as (never-null) but we would not touch any GHashTable API.

I do not expect that bindings would ever interpret (never-null) to mean anything at all because afaik bindings don't work with gpointer that is not already annotated with a (type) and in that case they know if NULL is allowed or not by checking for (nullable).

(In reply to comment #22)
> I was thinking more like:
> 
>  - a type of gpointer is always assumed to accept and return NULL unless
> either:
> 
>    - annotated with (type GObject) and not also with (nullable)
> 
>   or
> 
>    - explicitly annotated (never-null)
> 
> 
> 
> So we would annotate the return value of g_malloc() as (never-null) but we
> would not touch any GHashTable API.

Sounds good. A clarification as discussed on IRC: "annotated with (type GObject) …" should be "annotated with any (type) …".

> I do not expect that bindings would ever interpret (never-null) to mean
> anything at all because afaik bindings don't work with gpointer that is not
> already annotated with a (type) and in that case they know if NULL is allowed
> or not by checking for (nullable).

I've e-mailed various bindings people about this so hopefully they will comment.

FWIW from Vala's POV it doesn't really matter.  The GLib bindings aren't generated from GIR (and probably never will be), and until G-I supports generics other than those with built-in support (bug #639908) I don't think this will really matter outside of glib.

(never-null) might also be nice for some types which are always nullable by default (IIRC GCancellable and GError), though.

So it sounds like the only change being proposed is gpointers without type annotations will start defaulting to nullable and there is no change to gpointers with explicit type annotations?

In the Python bindings we already do this in a sense. Raw gpointers without type annotations are interpreted as integer values without any regard for nullability (0 always works). This is used for integer storage on things like GtkTreeIter::user_data.

The only thing that might be slightly concerning is the potential confusion for a developer adding a type annotation to a gpointer. This will have the side effect of also changing its nullability from nullable to never-null...

Just as another datapoint: from the point of view of the Haskell GI bindings treating gpointers without a type annotation as nullable is something that was implicitly done already (they were just represented in the bindings as Haskell Ptr types, which admit a null value), so this change will not change much.

It will have some effect in the generated API with the current binding generator, in that some function signatures will change from "Ptr ()" to "Maybe (Ptr ())", but I am not aware of any real world code that will be broken by this.

So, overall, no problems on the haskell-GI side with this change: the fallout will be small (if any), and conceptually it seems right.

Currently, the GirFFI bindings for Ruby ignore the nullable annotation, and assume all ingoing arguments can be null and all outgoing arguments and return values (of a pointer-like nature) may be null.

Apart from the special case of closure data, GirFFI also does handle untyped gpointers (as used in gi_marshalling_tests_pointer_in_return).

Whether return values and out parameters are nullable will probably not be relevant for GirFFI, since it's easier to always have the check for null present.

Once I get round to implementing the check for nullable for ingoing arguments, the particular default chosen in the annotations does not really matter I suppose, as long as g_arg_info_may_be_null always returns the correct value.

Created attachment 278832 [details] [review]
giscanner: Mark gpointer nodes as nullable by default

gpointer parameters and return types should be marked as nullable by
default, unless:
 • also annotated with (type) and not with (nullable); or
 • explicitly annotated with (never-null).

This introduces the (never-null) annotation as a direct opposite to
(nullable).

(In reply to comment #28)
> This introduces the (never-null) annotation as a direct opposite to
> (nullable).

We should do this in a more generic way:

  (nullable false)
  (not nullable)
  (!nullable)

etc. Even if we don't implement it for any other annotations right now, it at least is entirely obvious how we would if we decided to.

(In reply to comment #29)
> (In reply to comment #28)
> > This introduces the (never-null) annotation as a direct opposite to
> > (nullable).
> 
> We should do this in a more generic way:
> 
>   (nullable false)
>   (not nullable)
>   (!nullable)
> 
> etc. Even if we don't implement it for any other annotations right now, it at
> least is entirely obvious how we would if we decided to.

Fair point. Although we do already have things like (in) and (out) which should really be (direction in), (direction out), etc. Adding more syntax, like ‘!’, would require changes in gtk-doc and friends, which would cause a bit of disruption, but I guess it’s worthwhile in the long run.

My vote would be for (not nullable).

Other annotations which it might be worthwhile to allow inverting: (closure), (destroy), (optional), (foreign).

Created attachment 278869 [details] [review]
gfileutils: Mark the return value from g_path_skip_root() as nullable

It returns NULL for non-absolute paths.

Created attachment 279607 [details] [review]
gsignal: Mark the return value of g_signal_emitv() as (out) (optional)

Created attachment 294502 [details] [review]
gstring: Mark the return value from g_string_free() as nullable

It’s NULL iff free_segment is TRUE, so the annotation doesn’t quite
capture all the function definition, but is a safe over-estimate of the
return value’s nullability.

Created attachment 295728 [details] [review]
giscanner: Mark gpointer nodes as nullable by default

gpointer parameters and return types should be marked as nullable by
default, unless:
 • also annotated with (type) and not with (nullable); or
 • explicitly annotated with (not nullable).

This introduces the (not nullable) annotation as a direct opposite to
(nullable). In future, (not) could be extended to invert other
annotations.

Created attachment 295729 [details] [review]
giscanner: Ignore IOErrors from moving a temporary file

If this fails, the cache won’t work, but it’s only a cache.

Review of attachment 278869 [details] [review]:

Thanks.

Review of attachment 279607 [details] [review]:

Is this really correct?  We're not really allocating and returning a GValue here, so much as modifying one that was passed in...

Review of attachment 294502 [details] [review]:

Sure.  Makes sense.

Attachment 278869 [details] pushed as b9c94b3 - gfileutils: Mark the return value from g_path_skip_root() as nullable
Attachment 294502 [details] pushed as f829bde - gstring: Mark the return value from g_string_free() as nullable

Created attachment 298460 [details] [review]
gsignal: Mark the return value of g_signal_emitv() as (inout) (optional)

If we can get the default annotation changes for gpointers (attachment #295728 [details]) reviewed I’ll be able to update the original patch to remove the redundant (nullable) annotations for gpointers, and add (not nullable) annotations to keep the GIR file otherwise unchanged. Don’t want to do that until the g-ir-scanner changes are reviewed in case you don’t like the new syntax.

Review of attachment 295729 [details] [review]:

I'd prefer if patches like this mentioned what failed.  Is this something like breakage on Windows?  :+1: from me with that change.

Review of attachment 295728 [details] [review]:

This looks reasonable to me.

Review of attachment 298460 [details] [review]:

OK.

Comment on attachment 298460 [details] [review]
gsignal: Mark the return value of g_signal_emitv() as (inout) (optional)

Attachment 298460 [details] pushed as 073a81d - gsignal: Mark the return value of g_signal_emitv() as (inout) (optional)

(In reply to Colin Walters from comment #42)
> Review of attachment 295729 [details] [review] [review]:
> 
> I'd prefer if patches like this mentioned what failed.  Is this something
> like breakage on Windows?  :+1: from me with that change.

This was failing on Linux for me, though with a non-JHBuild build environment. I can't remember the circumstances more specifically than that, and didn't investigate properly at the time.

Attachment #295728 [details] and attachment #295729 [details] depend on attachment #278830 [details] from bug #729660 — would you mind reviewing that please? Rebasing/Disentangling the commits is not entirely trivial.

https://bugzilla.gnome.org/show_bug.cgi?id=729660#c5

patches don't apply cleanly anymore

(In reply to Philip Withnall from comment #46)
> Attachment #295728 [details] and attachment #295729 [details] depend on
> attachment #278830 [details] from bug #729660 — would you mind reviewing
> that please? Rebasing/Disentangling the commits is not entirely trivial.

I don’t think it’s worth rebasing until bug #729660 is fixed, since these annotation changes depend on it.

Matthias, would you be able to take a look at that please?

(In reply to Philip Withnall from comment #48)
> (In reply to Philip Withnall from comment #46)
> > Attachment #295728 [details] and attachment #295729 [details] depend on
> > attachment #278830 [details] from bug #729660 — would you mind reviewing
> > that please? Rebasing/Disentangling the commits is not entirely trivial.
> 
> I don’t think it’s worth rebasing until bug #729660 is fixed, since these
> annotation changes depend on it.
> 
> Matthias, would you be able to take a look at that please?

Actually, I’ll move those two GIR patches to bug #729660 and keep this bug for the glib changes, now that glib and gobject-introspection are separate Bugzilla products.

Comment on attachment 295729 [details] [review]
giscanner: Ignore IOErrors from moving a temporary file

Obsoleted by bug #751926.

Comment on attachment 295728 [details] [review]
giscanner: Mark gpointer nodes as nullable by default

Moved to bug #729660.

---

That leaves the patch to add missing (nullable) and (optional) annotations.  I’ll update that now to remove annotations which will become redundant with bug #729660, and to add the new (not nullable) annotation (from that bug) where appropriate to keep the GIR correct.

If the reviews for bug #729660 (once they happen) decide to go for a syntax other than ‘(not nullable)’, this patch will need rebasing again.

Created attachment 310502 [details] [review]
glib: Add missing (nullable) and (optional) annotations

Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.

Secondly, add various (not nullable) annotations as needed by the new
default in gobject-introspection of marking gpointers as (nullable). See
https://bugzilla.gnome.org/show_bug.cgi?id=729660.

This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.

Patch updated:
 • Rebased on master
 • Removed some (nullable) annotations which are made unnecessary by bug #729660
 • Added some (not nullable) annotations which are made necessary by bug #729660

Created attachment 310503 [details]
Diff of GLib-2.0.gir between master and the nullability changes

Created attachment 310504 [details]
Diff of Gio-2.0.gir between master and the nullability changes

Created attachment 310505 [details]
Diff of GObject-2.0.gir between master and the nullability changes

Here are three diffs which show the changes in the GLib GIR files between master of glib (7a65d1d3fb86b0ab46a0a425b79985e037cd3b68) and gobject-introspection (d1f62064ff2a570d8648c622e5b59f3a561f7e9a), and with the patches from this bug and bug #729660 applied on top.

Most of the changes are the automatic marking of @user_data parameters as nullable.

A few things to point out:
 • Whether the parameter to GFreeFunc should be nullable is an open question. It was previously not; now it is automatically marked as nullable.
 • Are keys for a GTree nullable? Previously they were not; now they have been automatically marked as such.
 • As well as the annotation changes, I have added some prose to the GSlice documentation to clarify its nullability behaviour. As an allocator, it’s a bit of a special case.

Review of attachment 310502 [details] [review]:

Do we actually parse "(not nullable)" today?  A quick check says we don't.

This is a large patch...can we pick this up at the start of the next release, or after branching?

(In reply to Colin Walters from comment #57)
> Review of attachment 310502 [details] [review] [review]:
> 
> Do we actually parse "(not nullable)" today?  A quick check says we don't.

Bug #729660.

> This is a large patch...can we pick this up at the start of the next
> release, or after branching?

Makes sense.

Now that bug #729660 has landed, this needs to be picked up again.

(In reply to Colin Walters from comment #57)
> This is a large patch...can we pick this up at the start of the next
> release, or after branching?

Colin? :-)

Review of attachment 310502 [details] [review]:

One question.

::: gio/ginputstream.c
@@ +125,3 @@
  * g_input_stream_read:
  * @stream: a #GInputStream.
+ * @buffer: (array length=count) (element-type guint8) (not nullable): a buffer

For these...we didn't end up doing what your commit message said, we only changed closures, right?

(In reply to Colin Walters from comment #61)
> Review of attachment 310502 [details] [review] [review]:
> 
> One question.
> 
> ::: gio/ginputstream.c
> @@ +125,3 @@
>   * g_input_stream_read:
>   * @stream: a #GInputStream.
> + * @buffer: (array length=count) (element-type guint8) (not nullable): a
> buffer
> 
> For these...we didn't end up doing what your commit message said, we only
> changed closures, right?

Not sure exactly what you mean — which commit message? In the case of g_input_stream_read(), the change to @buffer is to add (not nullable) to counteract the nullable-by-default gpointer convention.

Bug #729660 changed gpointer arguments and return types to be (nullable) by default unless they also have a (type) specified. This is intended to catch the `gpointer user_data` case. I believe that was OKed in bug #729660?

(In reply to Philip Withnall from comment #62)

> Bug #729660 changed gpointer arguments and return types to be (nullable) by
> default unless they also have a (type) specified. This is intended to catch
> the `gpointer user_data` case. I believe that was OKed in bug #729660?

Ah, right...but I think what threw me here is we're explicitly annotating this type as an `guint8[]`, which we should assume *can't* be nullable, right?

It looks like the code applies the (type) annotation before (nullable) so the annotation should be unnecessary, right?

(In reply to Colin Walters from comment #63)
> (In reply to Philip Withnall from comment #62)
> 
> > Bug #729660 changed gpointer arguments and return types to be (nullable) by
> > default unless they also have a (type) specified. This is intended to catch
> > the `gpointer user_data` case. I believe that was OKed in bug #729660?
> 
> Ah, right...but I think what threw me here is we're explicitly annotating
> this type as an `guint8[]`, which we should assume *can't* be nullable,
> right?
> 
> It looks like the code applies the (type) annotation before (nullable) so
> the annotation should be unnecessary, right?

Ah, I think this is caused by the difference between (type) and (element-type). I completely overlooked (element-type) when doing bug #729660. I'll put together a patch to mark gpointer arguments as non-nullable if they have (element-type) specified.

I've added some more tests to gobject-introspection:
   https://bugzilla.gnome.org/show_bug.cgi?id=757678
and it turns out that (element-type) implies (not nullable), as expected, so the annotation I added to g_input_stream_read() is redundant. I'll remove it from the patch.

Colin, do you have any other review comments, or thoughts about the questions in comment #56?

Created attachment 314979 [details] [review]
glib: Add missing (nullable) and (optional) annotations

Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.

Secondly, add various (not nullable) annotations as needed by the new
default in gobject-introspection of marking gpointers as (nullable). See
https://bugzilla.gnome.org/show_bug.cgi?id=729660.

This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.

> • Whether the parameter to GFreeFunc should be nullable is an open question. It was previously not; now it is automatically marked as nullable.

I think so.

> • Are keys for a GTree nullable? Previously they were not; now they have been automatically marked as such.

Looks like it, though `g_tree_lookup()` would be ambiguous just like hash tables, but we have `g_tree_lookup_extended()`.

> • As well as the annotation changes, I have added some prose to the GSlice documentation to clarify its nullability behaviour. As an allocator, it’s a bit of a special case.

Sounds good to me.


:+1: from me on your patch after that small change from comment #65.

Review of attachment 314979 [details] [review]:

https://en.wikipedia.org/wiki/Aircraft_marshalling#/media/File:Helicopter_hand_signal_move_forward.svg

\o/ \o/ \o/

Attachment 314979 [details] pushed as 25a7c81 - glib: Add missing (nullable) and (optional) annotations