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

Introduce boxes and vectors to reduce Result sizes. #3226

Closed
wants to merge 1 commit into from

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Nov 22, 2022

The following error variants were large enough to trip Clippy's result_large_err lint:

  • RenderPassCompatibilityError::IncompatibleColorAttachment
  • CreateShaderModuleError::Validation

Large error types are a problem, because they make the Result type large, requiring the program to copy around large numbers of bytes even in the success case. Since the failure case is rare, it's usually not a problem to just heap-allocate the contents to keep the Ok case small.

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

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.

Testing
Explain how this change is tested.

The following error variants were large enough to trip Clippy's
`result_large_err` lint:

- RenderPassCompatibilityError::IncompatibleColorAttachment
- CreateShaderModuleError::Validation

Large error types are a problem, because they make the `Result` type
large, requiring the program to copy around large numbers of bytes
even in the success case. Since the failure case is rare, it's usually
not a problem to just heap-allocate the contents to keep the `Ok` case
small.
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

Merging #3226 (dcb48dd) into master (04d12ba) will decrease coverage by 0.09%.
The diff coverage is 62.03%.

❗ Current head dcb48dd differs from pull request most recent head 76c0334. Consider uploading reports for the commit 76c0334 to get more accurate results

@@            Coverage Diff             @@
##           master    #3226      +/-   ##
==========================================
- Coverage   64.70%   64.61%   -0.10%     
==========================================
  Files          81       81              
  Lines       38819    39479     +660     
==========================================
+ Hits        25118    25509     +391     
- Misses      13701    13970     +269     
Impacted Files Coverage Δ
player/src/lib.rs 53.31% <ø> (ø)
wgpu-core/src/id.rs 81.25% <0.00%> (-2.63%) ⬇️
wgpu-core/src/lib.rs 96.55% <ø> (ø)
wgpu-core/src/pipeline.rs 21.11% <ø> (ø)
wgpu-core/src/present.rs 0.00% <0.00%> (ø)
wgpu-core/src/resource.rs 25.13% <ø> (ø)
wgpu-hal/src/dx11/device.rs 0.00% <0.00%> (ø)
wgpu-hal/src/empty.rs 0.00% <ø> (ø)
wgpu-hal/src/lib.rs 26.21% <ø> (ø)
wgpu/src/backend/direct.rs 54.80% <0.00%> (-0.30%) ⬇️
... and 44 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jimblandy
Copy link
Member Author

jimblandy commented Nov 22, 2022

The Android aarch64 CI failure is:

  Downloaded ndk-sys v0.4.0
error: failed to verify the checksum of `ndk-sys v0.4.0`
Error: Process completed with exit code 101.

@jimblandy jimblandy mentioned this pull request Nov 22, 2022
3 tasks
@kpreid
Copy link
Contributor

kpreid commented Nov 22, 2022

error: failed to verify the checksum of ndk-sys v0.4.0

FYI, my CI is seeing similar failures on macOS only, starting recently.

@Zageron
Copy link

Zageron commented Nov 23, 2022

Looks like a yank.

https://crates.io/crates/ndk-sys/versions

Not sure why this is causing failures on CI. (I am also seeing it)

@cwfitzgerald
Copy link
Member

Obsoleted by #3231

@jimblandy jimblandy deleted the large-errors branch November 23, 2022 23:57
@ErichDonGubler
Copy link
Member

@cwfitzgerald (CC @jimblandy): I don't think this PR is actually made obsolete by the rollback to MSRV 1.64. We will still need to confront the offending lint in an eventual upgrade to 1.65 or beyond, and in the meantime, this change is forwards-compatible with the current MSRV.

Is there a reason I'm missing for closing this?

@cwfitzgerald
Copy link
Member

@ErichDonGubler this PR was included as a commit on that PR.

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.

6 participants