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

Added is_interned method which returns a bool #57

Closed

Conversation

DanielJoyce
Copy link

  • Returns true if value already interned.
  • Added simple test as well.

* Returns true if value already interned.
* Added simple test as well.
@DanielJoyce
Copy link
Author

Closes #45

Comment on lines +243 to +245
pub fn is_interned<'a, Q: ?Sized + Eq + Hash + 'a>(val: &'a Q) -> bool
where
&'static T: Borrow<Q> + From<&'a Q>,
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: You can remove the From constraint for this method.

Suggested change
pub fn is_interned<'a, Q: ?Sized + Eq + Hash + 'a>(val: &'a Q) -> bool
where
&'static T: Borrow<Q> + From<&'a Q>,
pub fn is_interned<'a, Q: ?Sized + Eq + Hash + 'a>(val: &'a Q) -> bool
where
&'static T: Borrow<Q>,

Copy link
Author

Choose a reason for hiding this comment

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

If I do, then I need to specify generic bounds on line 72:

type annotations needed
cannot satisfy `_: std::cmp::Eq`rustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20%5B24%5D?24#file%3A%2F%2F%2Fhome%2Fdanieljoyce%2Fgit%2Fgeneral%2Finternment%2Fsrc%2Fintern.rs)
intern.rs(204, 9): required by a bound in `Intern::<T>::is_interned`
intern.rs(72, 19): consider specifying the generic argument: `::<T>`

Copy link
Author

Choose a reason for hiding this comment

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

Okay, one sec.

Copy link
Author

Choose a reason for hiding this comment

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

If I remove the From<> bounds on T, then it requires dismbiguation per the above error message.

Copy link
Author

Choose a reason for hiding this comment

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

If I move the is_interned to the impl<T: Eq + Hash + Send + Sync + 'static + ?Sized> Intern<T> { block, then

 let _ = Intern::new("Fortunato");
    assert!(Intern::is_interned(&"Fortunato"));
    assert!(Intern::is_interned("Fortunato"));

Both check as valid, but when I run the test, assert!(Intern::is_interned("Fortunato")); fails.

Comment on lines +65 to +66
assert!(Intern::is_interned(&"Fortunato"));
assert!(!Intern::is_interned(&"Montresor"));
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: Could you also test if we can pass just a &str to is_interned?

Suggested change
assert!(Intern::is_interned(&"Fortunato"));
assert!(!Intern::is_interned(&"Montresor"));
assert!(Intern::is_interned(&"Fortunato"));
assert!(!Intern::is_interned("Fortunato"));
assert!(!Intern::is_interned(&"Montresor"));
assert!(!Intern::is_interned("Montresor"));

Copy link
Author

@DanielJoyce DanielJoyce Aug 30, 2024

Choose a reason for hiding this comment

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

The problem is the && and & cases can't be unified trivially. Or I don't understand enough rust black magic to do so.

Basically Intern::is_interned(&"") checks Intern<&str> and Intern::is_interned("") checks Intern<str> which are different.

@droundy
Copy link
Owner

droundy commented Jul 19, 2024

Could you fix the conflicts, and also add an entry to CHANGELOG.md?

And thanks for the contribution!

@DanielJoyce
Copy link
Author

Whoops, will do, sorry, hadn't check back in a while. :)

@droundy
Copy link
Owner

droundy commented Sep 14, 2024

Due to conflict, I merged this outside of github. I also made some tweaks, so please take a look!

@droundy droundy closed this Sep 14, 2024
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