json-rust icon indicating copy to clipboard operation
json-rust copied to clipboard

Consider using `SmallString`

Open timothee-haudebourg opened this issue 5 years ago • 2 comments

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 avatar Oct 15 '20 21:10 timothee-haudebourg

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 avatar Oct 15 '20 21:10 timothee-haudebourg

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...

timothee-haudebourg avatar Oct 18 '20 09:10 timothee-haudebourg