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

Implement IntoValues and inner converter for Map #1225

Closed
tisonkun opened this issue Jan 6, 2025 · 2 comments · Fixed by #1226
Closed

Implement IntoValues and inner converter for Map #1225

tisonkun opened this issue Jan 6, 2025 · 2 comments · Fixed by #1226

Comments

@tisonkun
Copy link
Contributor

tisonkun commented Jan 6, 2025

Hi @dtolnay!

I'm using the map.rs file in my personal project today, and I made some changes that may be useful for the upstream:

impl<K, V> From<MapImpl<K, V>> for Map<K, V> {
    fn from(map: MapImpl<K, V>) -> Self {
        Map { map }
    }
}

impl Map<String, Value> {
    /// Consumes the `Map`, returning the underlying Map implementation.
    pub fn into_inner(self) -> MapImpl {
        self.map
    }

    /// Gets an iterator over the values of the map.
    #[inline]
    pub fn into_values(self) -> IntoValues {
        IntoValues {
            iter: self.map.into_values(),
        }
    }
}

/// An owning iterator over a serde_json::Map's values.
pub struct IntoValues {
    iter: IntoValuesImpl,
}

#[cfg(not(feature = "preserve_order"))]
type IntoValuesImpl = btree_map::IntoValues<String, Value>;
#[cfg(feature = "preserve_order")]
type IntoValuesImpl = indexmap::map::IntoValues<String, Value>;

delegate_iterator!((IntoValues) => Value);

I'm willing to send a PR if this is desired to be included in the upstream.

Besides, the clippy workaround for derivable_impls seems resolved now. I remove it and no more new warnings.

@dtolnay
Copy link
Member

dtolnay commented Jan 6, 2025

into_values would be fine. The other 2 APIs in your suggestion would be misguided to add in serde_json. Either of those makes indexmap upgrades such as #1031 only doable across a major version of serde_json which is too disruptive.

@tisonkun
Copy link
Contributor Author

tisonkun commented Jan 7, 2025

@dtolnay Thanks for your information and that's a good point!

Submitted a PR at #1226.

Out of the thread (for sharing):

  1. I found this patch when developing SPath (a JSONPath impl), while I later noticed that I can write a trait rather than making another Value struct.
  2. Such an into_xxx function reminds me of the other PR I made Impl String::into_chars rust-lang/rust#133057. I just often find that if we can take the ownership, we can save a clone. But generally speaking, "move"s are often/always "memcpy" in Rust?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants