🎨 zb: Remove unsafe type casting code#1439
Conversation
|
Cool. I'll look at it in more detail later but in the meantime, please split the commit to keep commits atomic. Here, I see at least 3 logical changes: removal of unneeded checks (which I'm not too sure about), bumping of MSRV and removal of unsafe code. |
|
@sander-machado ping? |
|
@sander-machado I think Rust 1.86 is not too new anymore. Let's bump the MSRV as part of this PR? You'll want to rebase though. |
Great I'll update my PR once that is done! |
I meant, you bump the MSRV as part of this PR but it's about to happen with another PR that's about to be merged: #1620. Please rebase on that. |
d643b8f to
638e74f
Compare
Yeah I was referring to that one (had to bump it 1 more anyway), as that's a coworker who made it. But yeah that's not obvious for you ofcourse haha. I have updated my commits to hopefully be atomic and tried to use to correct emojis. Btw I noticed that if you click the name of the emoji you copy the emoji string which almost made me use that instead before I noticed maybe that's why you keep getting people use the string? |
zeenix
left a comment
There was a problem hiding this comment.
LGTM apart from one thing and the usual nitpicks on commit messages: 😄
- They're missing the
zb:prefix. - second one could be shorter to be close to the guideline, something like
Replace unsafe downcast API w/ std's. - A description would be nice, even if a short one. For the second commit, you could use the same description as the PR's since this is the main commit.
- typo: build ins -> built-ins. However this becomes irrelevant if you change the title.
638e74f to
e6b126a
Compare
e6b126a to
6f2e8f4
Compare
Rustc 1.86 stabilizes a new feature called trait_upcasting. As quoted from the rust docs "Enable upcasts from dyn Trait1 to dyn Trait2 if Trait1 is a subtrait of Trait2". Which is exactly what this unsafe code was doing. Added the new syntax to enable this feature and remove these unsafe blocks. There were also multiple places where this check was called without any reason as it's checked at construction (and would just panic either way). Related to: rust-lang/rust#65991
These downcasts are already validated at construction time and would just panic either way, so they serve no purpose.
6f2e8f4 to
dc5194c
Compare
zeenix
left a comment
There was a problem hiding this comment.
The changes look good to me, but please use 🦺 as the emoji prefix and ensure that lines in description of git commit message < 75 chars (as per our and git conventions).
Rustc 1.86 stabilizes a new features called trait_upcasting. As quoted from the rust docs "Enable upcasts from dyn Trait1 to dyn Trait2 if Trait1 is a subtrait of Trait2". Which is exactly what this unsafe code was doing. I added the new syntax and increased the compiler version to 1.86 to enable this feature and remove these unsafe blocks. There were also multiple places where this check was called without any reason as it's checked at construction (and would just panic either way).
Related to: rust-lang/rust#65991