-
Notifications
You must be signed in to change notification settings - Fork 186
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
Builder next #146
Builder next #146
Conversation
9de7788
to
4ddce69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! A couple thoughts:
- Don't we want the
next
values to have the same lifetime checks as other pointers? - While it might be redundant to the unsafety of all(?) functions that process these structs, I think making the traits unsafe is appropriate to express the specific UB of passing in an incorrect one, bearing in mind that users may want to implement the structs manually to support extensions unknown to ash.
generator/src/lib.rs
Outdated
}; | ||
|
||
let next_trait = if has_next { | ||
quote!{pub trait #name_builder_next {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quote!{pub trait #name_builder_next {}} | |
quote!{pub unsafe trait #name_builder_next {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to this day (and I wrote about 20 000 lines of code in Rust for my bachelor thesis :D) I didn't know you could also mark a trait as unsafe. I leave this decision to @MaikKlein while I google away to learn about unsafe trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even think that a user would want to implement those traits manually, but if they do they better make sure it is valid, so yes it has to be unsafe.
Should this trait even be public?
Edit: Actually it probably doesn't need to be unsafe, because only the functions that use it need to be unsafe, but I think it is a good indicator to the user that they probably should be careful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the indication of the fact that it is unsafe to implement it.
In theory it shouldn't be public but I can see that someone may be stuck for some reason on older Ash version in future but may want to add an extension from newer Vulkan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's at least theoretically a population of users of unpublished extensions, too.
This looks fine to me, is there anything that you want to add? I am just asking because there is still WIP in the title. |
@MaikKlein Nope, just forgot to remove WIP :). But let me just rebase and squash. |
Looks like one of nv extensions broke the next : / |
do a |
|
This PR corresponds to #141.
Current implementation:
ApplicationInfoBuilder
->pub trait ApplicationInfoBuilderNext
extends
attribute provided in vk.xmlThis change is