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

Remove null terminators from other extensions #159

Merged

Conversation

rasmusgo
Copy link
Contributor

@rasmusgo rasmusgo commented May 7, 2024

The field other in ExtensionSet currently handles null terminated strings in a weird way. It adds null terminators in fn names but doesn't remove them in fn from_properties. I got bit by this when checking for extensions by name and not having null terminators in my strings. I think it would make the most sense to not have null terminators in Strings.

Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Good catch!

I think it would make the most sense to not have null terminators in Strings.

That makes sense to me. We'll need to insert nulls in fn names, then.

generator/src/main.rs Outdated Show resolved Hide resolved
@rasmusgo
Copy link
Contributor Author

rasmusgo commented May 8, 2024

Good catch!

I think it would make the most sense to not have null terminators in Strings.

That makes sense to me. We'll need to insert nulls in fn names, then.

It is already doing that.

@rasmusgo rasmusgo requested a review from Ralith May 15, 2024 20:15
@Ralith
Copy link
Owner

Ralith commented May 15, 2024

Revisiting this, there's a problem with the proposed approach: the most natural way to check for a name is to compare with xr::sys::WHATEVER_EXTENSION_NAME, which does include the null.

e: Maybe that's mitigated by those necessarily being the extensions that you can use the built-in bools for? Still feels inconsistent.

@rasmusgo
Copy link
Contributor Author

The other field is used for extensions not available as booleans and those don't have constants either. The names of those are copied from documentation or by listing available extensions. It's also not really a problem to have redundant null terminators in the extension names when listing required extensions. What bit me was that I was trying to have optional extensions by comparing my strings without null terminators with available extensions with null terminators.

@rasmusgo
Copy link
Contributor Author

The constants are also [u8] so I think it is fair that they contain \0 but the variables of type String doesn't.

@Ralith
Copy link
Owner

Ralith commented May 18, 2024

Fair enough.

generator/src/main.rs Outdated Show resolved Hide resolved
@rasmusgo rasmusgo requested a review from Ralith May 18, 2024 23:05
generator/src/main.rs Outdated Show resolved Hide resolved
openxr/src/generated.rs Outdated Show resolved Hide resolved
@rasmusgo rasmusgo requested a review from Ralith May 19, 2024 09:05
Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

@Ralith Ralith merged commit 155efe0 into Ralith:master May 19, 2024
6 checks passed
@rasmusgo rasmusgo deleted the remove-null-terminators-from-other-extensions branch May 19, 2024 20:31
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.

2 participants