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

examples: Replace one more unsafe transmute() with trivial ptr::cast() #351

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

MarijnS95
Copy link
Contributor

Noticed that one more pointer was being transmuted to another type rather than using a trivial cast, before unsafely dereferencing it.

This also removes the ptr::read() syntax introduced in #344 again, in favour of using a simple and more standard * deref.

@ErichDonGubler
Copy link
Member

I wanted to fix this PR up with a cargo fix && git commit --fixup HEAD, but...this PR isn't marked as editable by maintainership. 😅 Otherwise, this is good to go!

@ErichDonGubler ErichDonGubler self-assigned this Mar 12, 2025
@MarijnS95
Copy link
Contributor Author

I don't exactly see why maintainers always want to push to PRs, instead of waiting for the contributor to read the CI log themselves and address their mistakes 🤷 😅

…st()`

Noticed that one more pointer was being transmuted to another type
rather than using a trivial cast, before `unsafe`ly dereferencing it.

This also removes the `ptr::read()` syntax introduced in gfx-rs#344 again,
in favour of using a simple and more standard `*` deref.
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Mar 12, 2025

@MarijnS95: It's unclear what context you're referring to here (gfx-rs maintainership? metal-rs maintainership? The Rust community in general?), but the short answer is: I didn't want anyone to have to think about this trivial PR more than necessary. Removing an unused use statement so that CI passes seems uncontroversial, and much less interesting than getting the PR merged in the same day it was filed.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Mar 13, 2025

@ErichDonGubler it also happened as recently as #344 (comment) for way less trivial changes.

I've had terrible experiences with maintainers modifying my contributions in the past - specifically without attribution - and using their discretion to decide what constitutes a trivial change. In the most recent example, a BlueZ maintainer picked my patch without notification and completely refactored it to get rid of some duplication (that I was going to send a v2 for). I only found out a few months later when many user reports bisected broken headphone volume control to a patch on my name (and my name only 🙈), because of a logic bug introduced by the maintainers' refactor.

Hope that explains why I'm super wary of maintainers pushing into my patches 😰

Secondly, and this is subjective, it leaves a sense of helplessness: as if I'm somehow incapable of following up on a PR and seeing it through till the merge, such that a maintainer has to step in. Even if you're likely coming at this with best intentions 👍


But hopefully this PR is ready now, CI is green 🤞

@ErichDonGubler
Copy link
Member

I agree that pushing fixes that could possibly be controversial (independent of one's opinion of whether the changes are deserved or not) means that the contributor should confirm with the PR author, provided the PR author is responsive and trustworthy. To be fair, I did do that in #344, and you were able to choose whether to accept or reject them.

I don't have a reason to doubt your responsiveness or trustworthiness in this case, and I think that's important to tell you. 🙂

@ErichDonGubler ErichDonGubler merged commit cce2b48 into gfx-rs:master Mar 13, 2025
3 checks passed
@MarijnS95 MarijnS95 deleted the clippy branch March 13, 2025 13:59
@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Mar 13, 2025

@ErichDonGubler thanks for understanding! You did suggest changes that I could pick/apply separately indeed, but if I understand correctly only after discovering that you didn't have permissions to push them directly.

Note that I haven't intentionally turned that off, I think it is because I accidentally made a PR from an organization fork instead of my personal fork, and that likely has some more strict settings.

@ErichDonGubler
Copy link
Member

@ErichDonGubler thanks for understanding! You did suggest changes that I could pick/apply separately indeed, but if I understand correctly only after discovering that you didn't have permissions to push them directly.

I've found it's often easier to push easy commits (on top) of the commit stack in addition to the review feedback, and I usually do write my trivial feedback suggestions in a full IDE before writing the feedback in human language. It forces me as the reviewer to actually consider whether their changes are sensible enough to implement and compile, and it gives the PR author a Git-based affordance for handling the feedback.1

Footnotes

  1. It's also helpful in contexts where there are language gaps, but I don't think that that applies here.

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