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

Use associated constants for extension names #79

Closed
Ralith opened this issue Jul 18, 2018 · 9 comments
Closed

Use associated constants for extension names #79

Ralith opened this issue Jul 18, 2018 · 9 comments

Comments

@Ralith
Copy link
Collaborator

Ralith commented Jul 18, 2018

e.g. replace ash::extensions::Swapchain::name with ...::NAME being a constant of type &'static str. Simple consistency/clarity matter. I'd happily do this myself but, as always, I don't want to step on toes regarding/make changes mooted by the pending rewrite.

@MaikKlein
Copy link
Member

Yes that makes sense, also extension names need to be changed anyway because they really should reuse the generated extensions names from the vk.rs file.

I am sorry for blocking you on the generator branch. I don't have that much time right now, I thought I would have finished it last week but a few minor problems delayed it. I'll might find some time this week to finish it, if I don't encounter more problems.

@Ralith
Copy link
Collaborator Author

Ralith commented Jul 18, 2018

No worries, I'm not really blocked, I just feel a little bad opening issues instead of PRs.

@Ralith
Copy link
Collaborator Author

Ralith commented Aug 2, 2018

On review, I realized that the relevant type should probably be &'static CStr, and AFAIK there's no good way to get that in a constant yet.

@cheako
Copy link

cheako commented Jun 25, 2019

There is also the issue where the first thing you'll wanting to do with these names is compare to [i8; 256]s.
https://github.com/cheako/smithay/blob/492946a7779f187c73fcb2ab56640bbf14f48487/vkwayland/src/udev.rs#L108

A helper for this purpose would be appreciated.

@Ralith
Copy link
Collaborator Author

Ralith commented Jun 25, 2019

Your code would be a lot simpler and safer there if you constructed a CStr to compare against rather than calling out to libc.

@cheako
Copy link

cheako commented Jun 25, 2019

There is no fixed size array with null "padding" to CStr function. The CStr::from_bytes_with_nul() takes a slice and must end exactly with a trailing null. It's an error if the string is shorter than the slice intermediate representation, see FromBytesWithNulError::interior_nul. No, I'm satisfied that using libc's strncmp() here is the better option ""given all I can easily learn about CStr and CString.""

@Ralith
Copy link
Collaborator Author

Ralith commented Jun 25, 2019

For example, you can do unsafe { CStr::from_ptr(x.extension_name.as_ptr()) } == y.

@cheako
Copy link

cheako commented Jun 25, 2019

From strncmp() to sys::strlen(), I don't see this as an improvement. I could call str"n"len() and then CStr::from_bytes_with_nul_unchecked myself and it would be safer... but getting the length is going out of the way, it's better to just do the compare and get it over with. See KISS.

@MarijnS95
Copy link
Collaborator

It is now possible to do this since const-ness on CStr::from_bytes_with_nul_unchecked() has been stabilized in Rust 1.59, and we've adopted this in #715 which will be part of the next breaking Ash release.


For the unrelated from_ptr(...as_ptr()) case, there's now CStr::from_bytes_until_nul() (#746) which performs the nul character scan, and panics when going out of bounds on a slice view rather than unsafely reading increments of a pointer. Unfortunately this requires unsafely casting our c_char slice to a u8 slice (which is the same type on some platforms, i8 on others).

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

No branches or pull requests

4 participants