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

Refactor constructor safety #76

Merged
merged 7 commits into from
Oct 16, 2024
Merged

Refactor constructor safety #76

merged 7 commits into from
Oct 16, 2024

Conversation

asmello
Copy link
Collaborator

@asmello asmello commented Oct 14, 2024

Although a lot more inconvenient for us, some of our functions have invariants that have to be held externally, so they should be unsafe fn. This also makes it justifiable to expose them publicly (as then users can opt-in for manual checking of the invariants).

Notable changes:

  • Expose Pointer::new as a public (but unsafe) constructor (now called Pointer::new_unchecked). This gives users a way to bypass validation if they're sure they've got a valid string. Akin to String::from_utf8_unchecked and family.
  • Add PointerBuf::new_unchecked as a new unsafe constructor, to match Pointer::new_unchecked.
  • Made Token::from_encoded_unchecked unsafe. This one I didn't make public because I don't see much use for it. We may revisit this later.

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.0%. Comparing base (c96007e) to head (aeb33b1).

Additional details and impacted files
Files with missing lines Coverage Δ
src/pointer.rs 98.1% <100.0%> (+<0.1%) ⬆️
src/token.rs 100.0% <100.0%> (ø)

@asmello
Copy link
Collaborator Author

asmello commented Oct 14, 2024

I think the sanitizer test is failing due to a bug in the sanitizer itself (given the obscure error referencing its source code).

My guess is rust-lang/rust#111073

@asmello asmello requested a review from chanced October 14, 2024 23:01
@chanced
Copy link
Owner

chanced commented Oct 15, 2024

I'm at the fair with my family but noticed this come through and just wanted to say that I'm stoked about this one. I considered suggesting exposing Pointer::new but was hesitant.

I'll review both tomorrow morning.

Also, you're awesome dude.

@asmello
Copy link
Collaborator Author

asmello commented Oct 15, 2024

No rush, just came with a few ideas recently.

I noticed json-patch recently upgraded to 0.6.0, which is great (and I feel bad I let them do it, meant to contribute a PR myself).

@chanced
Copy link
Owner

chanced commented Oct 15, 2024

Hah, you did! Perhaps not to hatch directly but the state of this crate would be in much worse shape without your contributions. You brought it up a few levels! :)

Copy link
Owner

@chanced chanced left a comment

Choose a reason for hiding this comment

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

Ah, you went above and beyond with comments in this one :)

Looks great to me!

src/pointer.rs Outdated Show resolved Hide resolved
asmello and others added 2 commits October 16, 2024 17:07
@asmello
Copy link
Collaborator Author

asmello commented Oct 16, 2024

To be honest the unsafe for internal uses might be a bit overkill, I'm not 100% sure this is a great idea long term, but since it's an internal detail I think it's ok to experiment with.

@asmello asmello merged commit 9b77908 into chanced:main Oct 16, 2024
18 of 19 checks passed
@asmello asmello deleted the new-breaks branch October 16, 2024 16:19
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