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

Builder next #146

Merged
merged 1 commit into from
Nov 22, 2018
Merged

Builder next #146

merged 1 commit into from
Nov 22, 2018

Conversation

MatusT
Copy link

@MatusT MatusT commented Nov 16, 2018

This PR corresponds to #141.

Current implementation:

  • Each builder creates a trait ex.: ApplicationInfoBuilder -> pub trait ApplicationInfoBuilderNext
  • Structs implementing this trait are then taken in the builder's next function
  • Each struct then makes a bunch of impls for the traits based on the extends attribute provided in vk.xml

This change is Reviewable

@MatusT MatusT force-pushed the builder-next branch 3 times, most recently from 9de7788 to 4ddce69 Compare November 16, 2018 20:17
Copy link
Collaborator

@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.

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 Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
};

let next_trait = if has_next {
quote!{pub trait #name_builder_next {}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
quote!{pub trait #name_builder_next {}}
quote!{pub unsafe trait #name_builder_next {}}

Copy link
Author

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.

Copy link
Member

@MaikKlein MaikKlein Nov 16, 2018

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.

Copy link
Author

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.

Copy link
Collaborator

@Ralith Ralith Nov 18, 2018

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.

generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
@MaikKlein
Copy link
Member

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.

@MatusT MatusT changed the title WIP: Builder next Builder next Nov 22, 2018
@MatusT
Copy link
Author

MatusT commented Nov 22, 2018

@MaikKlein Nope, just forgot to remove WIP :). But let me just rebase and squash.

@MatusT MatusT changed the title Builder next WIP: Builder next Nov 22, 2018
@MatusT
Copy link
Author

MatusT commented Nov 22, 2018

Looks like one of nv extensions broke the next : /

@MaikKlein
Copy link
Member

do a git submodule update

@MatusT MatusT changed the title WIP: Builder next Builder next Nov 22, 2018
@MatusT
Copy link
Author

MatusT commented Nov 22, 2018

git submodule update wasn't enough because before rebasing it was on branch, I hate git. Anyway, all is solved and squashed.

@MaikKlein MaikKlein merged commit 1661cb8 into ash-rs:master Nov 22, 2018
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