Skip to content

Conversation

@raldone01
Copy link

The first commit fixes minor spelling mistakes I noticed.
The second one makes errors clone.
I have been writing a few crates using zerocopy and I frequently want to include SizeError with a mapped source in my error types but it is not Clone which is very inconvenient.

I rolled my own error type for now but it would be nice to just utilize the fancy printing zerocopy already does for errors.

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2025

Codecov Report

❌ Patch coverage is 18.64407% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.17%. Comparing base (e8700e6) to head (9e6dc73).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/error.rs 20.37% 43 Missing ⚠️
src/util/mod.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2715      +/-   ##
==========================================
- Coverage   88.91%   88.17%   -0.75%     
==========================================
  Files          20       20              
  Lines        5422     5470      +48     
==========================================
+ Hits         4821     4823       +2     
- Misses        601      647      +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@raldone01 raldone01 force-pushed the clone_size_error branch 2 times, most recently from ee6a7f0 to 04a213e Compare September 10, 2025 09:26
@raldone01 raldone01 changed the title Make errors clone Make errors Clone and lighten PartialEq bounds Sep 10, 2025
@raldone01 raldone01 changed the title Make errors Clone and lighten PartialEq bounds Make errors Clone and relax PartialEq bounds Sep 10, 2025
@raldone01 raldone01 changed the title Make errors Clone and relax PartialEq bounds Make errors Clone and relax PartialEq/Eq bounds Sep 12, 2025
@raldone01 raldone01 force-pushed the clone_size_error branch 2 times, most recently from 9c5b321 to 79ec664 Compare September 12, 2025 14:01
Manually implement PartialEq, Eq and Clone for types using SendSyncPhantomData.
This is necessary because the default derive imposes an additional PartialEq/Eq/Clone bounds on the phantom type.
Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks so much for the effort in putting it together!

I'm going to wait to merge until @jswrenn has also had a change to take a look, as he has stronger opinions about error code than I do.

@joshlf
Copy link
Member

joshlf commented Oct 16, 2025

/gemini review

@joshlf
Copy link
Member

joshlf commented Oct 16, 2025

@gemini Do you have any suggestions to make this PR better?

@joshlf
Copy link
Member

joshlf commented Oct 16, 2025

(@raldone01 I'm just using this PR to test our new gemini AI review integration – sorry for the noise)

@joshlf
Copy link
Member

joshlf commented Oct 16, 2025

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request makes the error types cloneable and relaxes their PartialEq and Eq bounds, which is a great improvement for usability. The implementation is correct and well-tested. I've also included some suggestions to simplify the code by using #[derive] for Clone, PartialEq, and Eq on the error structs, which is now possible due to the changes made to SendSyncPhantomData. The PR also includes several helpful typo fixes across the codebase. Overall, great work!

@joshlf
Copy link
Member

joshlf commented Oct 16, 2025

(Obviously ignore Gemini's very dumb suggestions.)

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