search

maciejhirsz / json-rust

JSON implementation in Rust
Apache License 2.0
563 stars 63 forks source link

Consider using `SmallString` #192

Open timothee-haudebourg opened 3 years ago

timothee-haudebourg commented 3 years ago

Hi,

For now you have two variants in JsonValue for strings (Short and String). This makes matching against json values tedious, since both variants are semantically equivalent. A better idea would be to use a single variant String(SmallString) where SmallString would be an owned string type similar to SmallVec. A similar problem occurs inside Object, where short keys are also optimized in the same way. In my IntoIter proposal (#191), the short string optimization is lost when items are returned with next because of the lack of a proper SmallString type.

I have found two crates providing exactly that, both based on SmallVec. The first one, smallstring, mentioned in #191, seems abandoned. However smallstr seems alive and widely used. Using this would allow you to unify both string variants in JsonValue and allow the IntoIter iterator to yield items of type (SmallString, JsonValue) instead of (String, JsonValue).

I understand that this would break the compatibility with older versions because the JsonValue api would drastically change, but I think it is necessary to have a simpler and more flexible interface (and implementation).

timothee-haudebourg commented 3 years ago

Since you seem to be avoiding having any dependencies, you could at least recycle the Key type used in Object, rename it to something like SmallString, make it public, and use it to represent object keys and JsonValue string variants in a uniform manner.

timothee-haudebourg commented 3 years ago

I see you are planning to use Cow<str>, but I don't really understand the benefits. I can see some use cases where it would be interesting to have some borrowed keys and values, but on the other hand you lose the small strings optimization. Ideally a Cow<str>-like structure with SmallString as owned type would be nice...