Skip to content

Remove unsafe mem::transmute calls#801

Open
joshlf wants to merge 1 commit intowinnow-rs:mainfrom
joshlf:remove-transmute
Open

Remove unsafe mem::transmute calls#801
joshlf wants to merge 1 commit intowinnow-rs:mainfrom
joshlf:remove-transmute

Conversation

@joshlf
Copy link

@joshlf joshlf commented Jul 8, 2025

No description provided.

@epage
Copy link
Collaborator

epage commented Jul 8, 2025

Is there a concern with the unsafe calls besides it being unsafe? I feel like this doesn't seem to justify the cost of a dependency.

@joshlf
Copy link
Author

joshlf commented Jul 8, 2025

The only concern would be future-proofing against changes that affect the layout; with this change, those would be detected and compile failures. But it's a minor concern; definitely a judgment call as to whether it's worth taking a dependency for.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 16151580562

Details

  • 2 of 8 (25.0%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.001%) to 46.632%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/stream/bstr.rs 1 4 25.0%
src/stream/bytes.rs 1 4 25.0%
Files with Coverage Reduction New Missed Lines %
src/error.rs 2 27.82%
Totals Coverage Status
Change from base Build 16149430809: -0.001%
Covered Lines: 1592
Relevant Lines: 3414

💛 - Coveralls

@epage
Copy link
Collaborator

epage commented Jul 8, 2025

I copied these from bstr and just looked to confirm what they do and apparently the transmute can be replaced with a cast, see BurntSushi/bstr@ef95d88. I'd be open to updating to match bstr.

@joshlf
Copy link
Author

joshlf commented Jul 8, 2025

I copied these from bstr and just looked to confirm what they do and apparently the transmute can be replaced with a cast, see BurntSushi/bstr@ef95d88. I'd be open to updating to match bstr.

That retains the same fundamental concern about sensitivity to layout, but I agree that a cast is marginally better than a transmute. If you're going to go that route, I'd suggest the following variant, which avoids an as cast:

let slice: *const [u8] = slice;
unsafe { &*(slice as *const BStr) }

@epage
Copy link
Collaborator

epage commented Jul 8, 2025

I'd prefer to copy bstr verbatim rather than us paving our own path.

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

Comments