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

Basic types #1363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Basic types #1363

wants to merge 2 commits into from

Conversation

filnet
Copy link
Contributor

@filnet filnet commented Jun 2, 2022

This PR is a follow up of #1134. Not sure what is the accepted way of contributing to someone else's PR.

Changes include:

So a bit more generic and the const VULKAN: &[&str] = &[...] is gone. :)

@filnet
Copy link
Contributor Author

filnet commented Jun 2, 2022

A next step would be to address the Vulkan specific hack.

We need a cleaner way to trigger the "special" handling of Vulkan records:
1/ could be based on a new flag in the Record itself (or based on the state of the Record)
2/ could be globally enabled in the Vulkan-1.0.gir file (via meta data or something else)
3/ could be globally enabled in the consumer Gir.toml file when declaring the external library
4/ could be enabled in gir itself (like now but in a "better" way via extension/plugin/what not).

src/parser.rs Outdated
@@ -264,6 +265,13 @@ impl Library {
) -> Result<(), String> {
if let Some(typ) = self.read_record(parser, ns_id, elem, None, None)? {
let name = typ.get_name();
// TODO: Don't hardcode to "Vulkan"
let typ = if self.namespace(ns_id).name.eq("Vulkan") {
assert!(typ.is_incomplete(self));
Copy link
Contributor Author

@filnet filnet Jun 2, 2022

Choose a reason for hiding this comment

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

We probably won't keep this assert but it shows that Vulkan records are incomplete.
Unfortunately, non Vulkan records can be incomplete too so we can't use is_incomplete as the trigger for the new behavior.

And then there is the problem of the "ash::vk::{}" format string: where should it be specified if we want to externalize it (to make gir Vulkan agnostic again).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative approach to mapping the names with "ash::vk::{}" would be to not do it and, instead, generate rust type aliases in a post process step.

pub type vulkan::Bool32 = VkBool32;

Advantage is that the Vulkan namespace will not be erased anymore (less "magic").

Disadvantage is that a bit more rust code will be generated.

Copy link
Contributor Author

@filnet filnet Jun 2, 2022

Choose a reason for hiding this comment

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

Likewise, instead of tweaking the Vulkan namespace on the fly, we could introduce a postprocess hook that tweaks/transforms the namespace after it was parsed.

@filnet filnet force-pushed the basic_types branch 2 times, most recently from 7746d63 to e4bc4ca Compare June 2, 2022 21:53
@filnet
Copy link
Contributor Author

filnet commented Jun 2, 2022

I went with post processing the Vulkan namespace.
And found src\custom_type_glib_priority.rs as example to follow.

also rename the new Basic::Vulkan enum value to Basic::Typedef
@bilelmoussaoui
Copy link
Member

What is the status of this one?

@MarijnS95
Copy link
Contributor

@bilelmoussaoui I don't think we ever continued the discussion how to best map Vulkan-1.0.gir (not GstVulkan-1.0.gir) fundamental types in gir.

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.

3 participants