gir.core icon indicating copy to clipboard operation
gir.core copied to clipboard

Methods with nullable fundamental return types should return null

Open hol430 opened this issue 2 years ago • 6 comments

Resolves #867

  • [x] I agree that my contribution may be licensed either under MIT or any version of LGPL license.

~(Edit: Please don't merge yet, I still need to add some tests.)~

hol430 avatar May 12 '23 06:05 hol430

Thank you for the PR. Great to see, that you want to add tests.

Before I start a short disclaimer: I'm not very familar with fundamental types. My following remarks could be obviously wrong without me knowing it. Please correct me if I'm wrong.

Are you sure this is a minimal implementation for a fundamental type? There are functions like value_fundamental_tester_collect_value or value_fundamental_tester_free_value where I wonder if they are really necessary. From the documentation it sounds like a fundamental type is something like gboolean. For testing purposes you could create something like gboolean2 which is hopefully pretty simple.

I would suggest to put the definition of the fundamental type into the GirTestLib/data directory and use it in the tester class. The tester class itself looks quite complex, too. As a reference see for example girtest-alias-tester.h and girtest-alias-tester.c. Such a class could just have some methods which return a fundamental type.

Did you see the docs for this topic (https://docs.gtk.org/gobject/concepts.html#non-instantiatable-non-classed-fundamental-types)

badcel avatar Jul 27 '23 18:07 badcel

Yeah this is definitely not a minimal fundamental type. I initially implemented something similar to gchar (using the document that you linked), but I realised that I actually want an intantiatable fundamental type for my tests, as that's the use case that I want to reproduce.

I couldn't find an easy way to do that without implementing a reference counter, so I ended up with what we have here, which is loosely based on the gobject implementation. The collect_value function is required in this case (gir complains if it's missing), and although the free_value function could be omitted, I left it in for completeness, in case this fundamental type is used for other tests in the future where it's needed.

Thanks for the tip about the GirTestLib/data directory; I hadn't found that, but that's a great idea to separate it from the tester class.

hol430 avatar Jul 28 '23 04:07 hol430

Which real world fundamental type do you have at hand which is instantiatable so it makes sense to have a test for it?

(GObject is not marked as fundamental in the gir file.)

Actually the generator currently is not differentiating between instantiatable and non instantiatable fundamental types. As far as I know there is no attribute in the gir file for it?

According to the docs I would even doubt that there is something like this?

badcel avatar Jul 28 '23 07:07 badcel

In my case it was a GdkEvent. I was calling a method which returns a GdkEvent in some cases, and NULL in some other cases. What I observed was that when NULL was returned on the native side, the bindings were giving me a GdkEvent wrapper around IntPtr.Zero, which is a bit misleading as you might imagine :).

hol430 avatar Jul 29 '23 07:07 hol430

Yep this is misleading.

Do you think it makes a difference wether we define something like gchar2 vs something like GdkEvent from a testing perspective?

From my point of view it should be the same if we have some c api which creates such types.

So the question would be why didn't you continue to work on some fundamental type like gchar (which I hope would be simpler than something like GdkEvent).

badcel avatar Jul 29 '23 09:07 badcel

The Regress library from the GObject Introspection test suite has an implementation of a fundamental type that is used on their tests RegressTestFundamentalObject as well as several subclasses: https://gitlab.gnome.org/GNOME/gobject-introspection/-/blob/main/tests/scanner/regress.h?ref_type=heads#L1037 It's a good reference for a simple implementation.

ylatuya avatar Oct 26 '23 13:10 ylatuya

Superseeded by #1056

badcel avatar May 07 '24 19:05 badcel