Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GDNative] refactored API struct into core and extensions #12779

Merged

Conversation

karroffel
Copy link
Contributor

No description provided.

@@ -33,14 +33,39 @@ def _build_gdnative_api_struct_header(api):
'',
'#ifdef __cplusplus',
'extern "C" {',
'#endif',
'#endif'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comma is still needed here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, for some reason it's not an error so I didn't notice. Will fix.

'typedef struct godot_gdnative_api_struct {',
'\tvoid *next;',
'\tconst char *version;',
'\tunsigned int type;'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comma?

@karroffel karroffel force-pushed the gdnative-api-struct-refactor branch from d613125 to 2248420 Compare November 9, 2017 15:24
@karroffel karroffel force-pushed the gdnative-api-struct-refactor branch from 2248420 to 983404e Compare November 9, 2017 17:39
@akien-mga akien-mga merged commit 0de6cba into godotengine:master Nov 9, 2017
@endragor
Copy link
Contributor

Is there a point in having three-number version for APIs instead of just one number? How do you plan to version the APIs, i.e. what would semantically a change of each number mean?

@endragor
Copy link
Contributor

endragor commented Nov 10, 2017

Also it probably makes sense to extract fields type and next into a separate struct (godot_gdnative_api_header). Because, if I'm not mistaken, those two first fields in specific API structs may be aligned differently between different structs, so you may not be able to check the type properly.

@endragor
Copy link
Contributor

We also discussed that binding libraries should have a way to report an error about unknown version (or API type in this case). Could you please add that?

@karroffel
Copy link
Contributor Author

Is there a point in having three-number version for APIs instead of just one number? How do you plan to version the APIs, i.e. what would semantically a change of each number mean?

Two numbers should be enough, yeah. It should be using semantic versioning. If we add more functions to the core or extensions then the patch minor version would be bumped. Major would be bumped if compatibility gets broken. So 3.0.1 might have a core version 1.1 while Godot 4.0 will probably have 2.0 again.

Because, if I'm not mistaken, those two first fields in specific API structs may be aligned differently between different structs, so you may not be able to check the type properly.

No, those fields should be the same since the struct are not annotated to be packed, so all compatible C compilers will align the fields all in the same places.

We also discussed that binding libraries should have a way to report an error about unknown version (or API type in this case). Could you please add that?

Yes, will add that, thanks for reminding me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants