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

Create FenderString type #93

Open
boxbeam opened this issue Apr 17, 2023 · 6 comments
Open

Create FenderString type #93

boxbeam opened this issue Apr 17, 2023 · 6 comments
Assignees

Comments

@boxbeam
Copy link
Contributor

boxbeam commented Apr 17, 2023

Our current string type uses a rust string, which can't be indexed by character. We should switch to a string type which holds a Vec<char>.

@FuzzyNovaGoblin
Copy link
Contributor

this is done and an optimization was added to increase the performance of modifying strings by updating the cached version of the string as well but this could cause a performance hit in some scenarios so we should do some bench-marking to see if this is a good idea or not

since this is a further and technically unnecessary optimization I believe we can leave it to after the beta launch as more of a improving the language steps rather than just creating the initial version of it

@FuzzyNovaGoblin
Copy link
Contributor

possible solutions are:

  • separate the two forms into separate functions giving the control to the user so they can use what is best for their scenario
    • basically one set of functions would work like a string builder where the actual string needs to get built at a later time
    • and the other would reflect just modifying a string, like in rust
  • have it be a feature to enable the other form of modification
  • a mix of both where there is a feature to set the default function to be used but either could be used if specified
  • so there are 3 functions, each of the 2 forms and one that is an alias to one of them

I feel like we should go with either the first or third and then if we go with the third say something in the docs that the default form should only be used in the repl or maybe personal scripts and that any code that gets shared should use the specific forms to maintain performance

this might also make no difference and its just faster to never do it in which case im putting too much thought into this thing that I am saying we shouldn't think about right now anyway

@boxbeam
Copy link
Contributor Author

boxbeam commented May 26, 2023

I think storing strings strictly as Vec<char> with no other representation will be the best long term solution. It would also allow us to create a substring type so you could get a substring of a string in O(1). I would be willing to help (re)write functions to support this.

@FuzzyNovaGoblin
Copy link
Contributor

Well with the current implementation and the potential optimization you can do that

cause the strings are just a Vec but there is a cached version of the printable version of the string so that if you print the same piece of text multiple times you don't need to iterate over the Vec each time

the optimization I am talking about is if the cached string is up to date and another string or character gets appended then it will also append the character to the cached rust string

@FuzzyNovaGoblin
Copy link
Contributor

the more I think about it the more ways I can see for optimizing it in different ways and for prioritizing different things, like compute time vs memory. So I wanna say push off trying to do all these now so we can get a better idea of what will be usable and work best in real scenarios during the beta. So for now lets just either have the cached string get updated when quickly possible or just remove it for now and just have it get dropped when the cached string is de-synced.
Then at a later time after evaluating it in the real world we can decide if that was the right call, or if the cached string should just not exist at all, or if you should have to specify if you want to have one or not when creating the string so that short and constantly reprinted strings can be cached; but longer, and constantly being modified strings will just never cache their printable forms.

@FuzzyNovaGoblin
Copy link
Contributor

actually having the caching be an optional opt in thing sounds like a good idea to just implement now if you agree

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

2 participants