+
Skip to content

Conversation

hol430
Copy link
Contributor

@hol430 hol430 commented May 12, 2023

Resolves #867

  • 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 hol430 marked this pull request as draft May 12, 2023 06:07
@hol430 hol430 force-pushed the bug/NullRecord branch 2 times, most recently from 5907f49 to cb25ef0 Compare May 25, 2023 05:19
@hol430 hol430 marked this pull request as ready for review July 27, 2023 01:38
@hol430 hol430 changed the title Methods with nullable return types should return null Methods with nullable fundamental return types should return null Jul 27, 2023
@hol430 hol430 force-pushed the bug/NullRecord branch 2 times, most recently from 0321aee to 25cf877 Compare July 27, 2023 01:55
@badcel
Copy link
Member

badcel commented Jul 27, 2023

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)

...if the pointer returned from native code is NULL. Previously we would
just create a managed object around a NULL handle. I've also written
unit tests to verify this behaviour.

I've also slightly modified the layout of the record return type to
managed expression class, to make it more consistent with the other
converters.
@hol430
Copy link
Contributor Author

hol430 commented Jul 28, 2023

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.

@badcel
Copy link
Member

badcel commented Jul 28, 2023

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?

@hol430
Copy link
Contributor Author

hol430 commented Jul 29, 2023

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 :).

@badcel
Copy link
Member

badcel commented Jul 29, 2023

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).

@ylatuya
Copy link

ylatuya commented Oct 26, 2023

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.

@badcel
Copy link
Member

badcel commented May 7, 2024

Superseeded by #1056

@badcel badcel closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Methods returning records never return null

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载