refactor(dx12): remove unsafe ops in Adapter::texture_format_capabilities
#3194
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cwfitzgerald
reviewed
Nov 9, 2022
| &mut data_no_depth as *mut _ as *mut _, | ||
| mem::size_of::<d3d12::D3D12_FEATURE_DATA_FORMAT_SUPPORT>() as _, | ||
| ptr::addr_of_mut!(data_no_depth).cast::<c_void>(), | ||
| DWORD::try_from(mem::size_of::<d3d12::D3D12_FEATURE_DATA_FORMAT_SUPPORT>()) |
There was a problem hiding this comment.
My only hesitation is the verbosity of this
There was a problem hiding this comment.
Coming back to this with a fresh brain:
- RE: pointer slinging:
- We can definitely omit the explicit
::<c_void>. I've done so with 7c46b4b. - We could fully qualify the
addr_of_mutsymbol, which would make this shorter and, IMO, easier to follow. In general, I think a lot of this code would be smaller with no loss of clarity (and therefore more friendly to the mental cache) without path qualifications for most symbols.
- We can definitely omit the explicit
- RE:
size_of: I've seen folks like @retep998 make utility crates for Windows specifically to convertsize_of'susizes toDWORDs. Maybe adding something like an extension trait for this (in a separate PR) would be good?
Thoughts?
There was a problem hiding this comment.
Honestly now that I look at this, this seems fine.
ErichDonGubler
force-pushed
the
d3d12-format-support-safety
branch
from
7669de0 to
7c46b4b
Compare
ErichDonGubler
changed the title
unsafe ops in Adapter::texture_format_capabilitiesunsafe ops in Adapter::texture_format_capabilities
ErichDonGubler
force-pushed
the
d3d12-format-support-safety
branch
2 times, most recently
from
c11fb89 to
9181af2
Compare
ErichDonGubler
force-pushed
the
d3d12-format-support-safety
branch
from
9181af2 to
4d91332
Compare
|
The ...where I don't think that this failure is related to this change, but I'm not positive. Do you know, @cwfitzgerald? |
|
Yeah, llvmpipe has been crapping the bed on the multi-threaded test of late. |
cwfitzgerald
approved these changes
Nov 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
cargo clippy.RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknownif applicable.Add change to CHANGELOG.md. See simple instructions inside file.I don't think that this is necessary.Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories
Description
Describe what problem this is solving, and how it's solved.
The code being changed here used
unsafeandascasting unnecessarily. Both are unusually fraught parts of writing in Rust. So, where possible, we'd like to avoid them. In this case…we can!Testing
Explain how this change is tested.
It still compiles, and I feel confident that the changes are correct, but I haven't tested locally. I can test this locally, though, if you wish.