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

Unify Resolved*Descriptors into *Descriptors #7079

Merged
merged 11 commits into from
Feb 7, 2025

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Feb 7, 2025

Connections
Continuation of #7056, based on my idea from #5121 (comment)

Description
Generalize Resolved*Descriptors into *Descriptors using generics. The only difference is id->Arc and some bounds mambo jumbo.

All commits are standalone.

Testing
Fearless refactoring is provided by Rust.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@sagudev sagudev marked this pull request as ready for review February 7, 2025 12:25
@sagudev sagudev requested review from crowlKats and a team as code owners February 7, 2025 12:25
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

I think this is a great change in general, have some nitpicks.

If possible, could you modify the commit messages to say Unify NameOfStruct into NameOfStruct or so, so that they make sense outside of the context of this PR, so I can rebase-and-merge.

<[BufferBinding<B>] as ToOwned>::Owned: std::fmt::Debug,
<[S] as ToOwned>::Owned: std::fmt::Debug,
<[TV] as ToOwned>::Owned: std::fmt::Debug,
[BindGroupEntry<'a, B, S, TV, T>]: ToOwned,
Copy link
Member

Choose a reason for hiding this comment

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

Thought: I wonder if it would be worthwhile (not in this PR) to roll our own Cow type without this bound, as we don't need it at all.

@sagudev
Copy link
Contributor Author

sagudev commented Feb 7, 2025

I think this is great opportunity for me to try jj for editing commits, because editing them in git is pain.

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
…scriptor`

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
…ptor`

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
@cwfitzgerald
Copy link
Member

@sagudev this good to land?

@sagudev
Copy link
Contributor Author

sagudev commented Feb 7, 2025

@sagudev this good to land?

Yes.

BTW jj did good, except for pushing where I fallback to git.

@cwfitzgerald cwfitzgerald merged commit 4675393 into gfx-rs:trunk Feb 7, 2025
33 checks passed
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