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

Consider using SmallString #192

Open
timothee-haudebourg opened this issue Oct 15, 2020 · 2 comments
Open

Consider using SmallString #192

timothee-haudebourg opened this issue Oct 15, 2020 · 2 comments

Comments

@timothee-haudebourg
Copy link

timothee-haudebourg commented Oct 15, 2020

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 timothee-haudebourg changed the title Consider using ShortString Consider using SmallString Oct 15, 2020
@timothee-haudebourg
Copy link
Author

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
Copy link
Author

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

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

No branches or pull requests

1 participant