Support non-str values in a Rodeo (yet another attempt)#52
Open
anchpop wants to merge 1 commit intoKixiron:masterfrom
Open
Support non-str values in a Rodeo (yet another attempt)#52anchpop wants to merge 1 commit intoKixiron:masterfrom
anchpop wants to merge 1 commit intoKixiron:masterfrom
Conversation
44a078a to
eeff631
Compare
eeff631 to
c001df0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is attempting the same thing as #32. The goal is the same but the implementation is different. For example, #32 is actually unsound, while this PR is not (to my knowledge). The critical difference is that Spur must be typed with the type being stored. This prevents you from using a Spur from a Rodeo storing Strings with a Rodeo storing something else.
Another difference is that the
Internabletrait is designed to be implemented for the owned version of the type, e.g.StringorVec<u8>. This allows the user to take the owned value,Vec<u8>, and spur-ify it by doingSpur<Vec<u8>>. This is imo more intuitive. A downside is that the type of the Rodeo cannot be inferred from the types being inserted into it.The issue brought up by @Kixiron, "We can't assume that all internable types play nicely as byte slices", is not really addressed by this PR. My goal was not to intern OsStr, but to intern Vecs of types that implement Copy.
I put the type being stored as the first type argument to Rodeo. This is a breaking change obviously. Anyone using a non-default Spur type will need to update their code. We could probably use
diagnostic::on_unimplementedto make it clear to users what they need to do when upgrading.I also added an example of the new functionality. IMO, it is very intuitive:
Disclaimer: Some LLM assistance was used in the creation of this PR. I updated Rodeo and Arena, then started updating RodeoReader and RodeoResolver, then used LLMs to automate the mechanical work of updating all the types in RodeoReader RodeoResolver. The design is mine and I've reviewed all the code manually of course. I also left many references to strings in the code (e.g. variable names and comments) as IMO focusing on the concrete case makes it easier to understand.