Skip to content

tests: Just use unwrap()#384

Merged
alexcrichton merged 1 commit into
composefs:mainfrom
cgwalters:use-result-not-macro-t
Oct 24, 2024
Merged

tests: Just use unwrap()#384
alexcrichton merged 1 commit into
composefs:mainfrom
cgwalters:use-result-not-macro-t

Conversation

@cgwalters
Copy link
Copy Markdown
Collaborator

I don't think this unique-to-us t! macro is gaining anything really over just calling unwrap() directly which is way more widely used and idiomatic.

I only converted a few testing functions to start momentum and hopefully new code can avoid t!.

I don't think this unique-to-us `t!` macro is gaining anything
really over just calling `unwrap()` directly which is *way*
more widely used and idiomatic.

I only converted a few testing functions to start momentum
and hopefully new code can avoid `t!`.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters requested a review from xzfc October 21, 2024 22:30
@alexcrichton
Copy link
Copy Markdown
Collaborator

Heh if you're curious for the backstory on this: long ago we had try!(foo()) instead of foo()? so t!(foo()) was sort of an equivalent to that. Additionally .unwrap() wouldn't report the line number of the caller, it'd report the line number in the standard library. So long ago it was both more useful and more idiomatic. Nowadays though it's neither, so it's great to see it removed!

@alexcrichton alexcrichton merged commit e0e69f6 into composefs:main Oct 24, 2024
@cgwalters
Copy link
Copy Markdown
Collaborator Author

cgwalters commented Oct 24, 2024

That makes total sense, I am definitely aware of the age of the crate. I actually started learning Rust in the age of try! so I'm familiar 😄 I think my first nontrivial Rust code was in this commit from 2016.

Actually of potential interest to you and also topically relevant for future directions of this crate: I remember skimming through the source code of this crate at the time thinking "what does a tar parser in Rust look like" and being quite surprised at the usage of unsafe and wondering why it was needed...shouldn't the whole premise of Rust be that one can write a tar parser without unsafe?

I now know enough to understand that technically today we could rewrite it with just slicing a [u8] and not have unsafe, but at a loss of ergonomics. But as of lately there's the project safe transmute which is what we really want of course...but it's still nightly only.

Any opinions on adding a dep on say zerocopy and adding #[deny(unsafe_code)] to this crate?

@alexcrichton
Copy link
Copy Markdown
Collaborator

Heh good point! Agreed it would be best to remove all the unsafe nowadays.

My only hesitation on zerocopy itself (I don't have a ton of experience with it prior) is that it looks like it's got major version bumps every so often. That would likely become a public dependency of the tar crate meaning that if we were to update the dependency then it would force a major version bump for tar which may not otherwise be necessary. Maybe it's possible though to make zerocopy a private dependency with some refactoring in tar? (unsure)

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.

2 participants