[WIP][half working][for reference][3 attempts] Implement mutable ::String#2713
[WIP][half working][for reference][3 attempts] Implement mutable ::String#2713janbiedermann wants to merge 3 commits intoopal:masterfrom
Conversation
|
Specs are expected to fail at this time. Only 23 failures, some due to different #frozen? semantics. And at the same time the first String#<< and String#concat specs passing! |
opal/corelib/string.rb
Outdated
| proto.append = function(s) { return this.value += s.toString(); } | ||
|
|
||
| // Make MutableString globally available. | ||
| Opal.global.MutableString = MutableString; |
There was a problem hiding this comment.
Do we need it on global? Or is it just a convenience?
There was a problem hiding this comment.
if the bridge would work perfectly i think it would be nice to have a shortcut for internal use, like new Map() for ::Hash. But currently its a convenience for debugging.
opal/corelib/string.rb
Outdated
| # used with some underlying "magic". So even internally within Opal its preferrable | ||
| # to use Opal.String.$new() over new MutableString(). Best practice is not to use | ||
| # MutableString directly at all, except for debugging. | ||
| class String < `MutableString` |
There was a problem hiding this comment.
This seems pluggable. Can we perhaps do corelib/string/mutable.rb ?
There was a problem hiding this comment.
Probably, i will experiment further.
| require 'opal' | ||
| puts 'ready' | ||
|
|
||
| t = String.new('bla') |
There was a problem hiding this comment.
So, this is the only way we can get a mutable String, right? Perhaps we can also do String#+, maybe String#dup and maybe also by default for dstr (ie. expressions like "test#{other}"). If it can be pluggable, then we would just do String.new on it.
There was a problem hiding this comment.
Yes, currently its the only way. So everywhere we return a new_string it should be ::String.new(new_string)
|
Overall, nice implementation, but it feels kinda hacky to me. Perhaps we could get rid of some hacks? Also I left a couple of comments. |
…rove MutableString implementation
|
@hmdne Thanks for looking over it! I agree, its a bit hacky. So this iteration improves Opal.bridge to work with arbitrary JS Classes, so also with MutableString. It also improves MutableString to be a real JS class. Its a bit less hacky now. Its not yet pluggable, i still want to experiment a bit "within the same realm". |
|
This is maybe is so far the least hacky approach, but still hacky. It removes the shadowed Ruby Class, uses a nice JS Class, and a Proxy ... although it solves some problems of the previous attempts, it creates new ones. Main Problem is now, need to take care to return the Proxy to keep things working. However, the impact on asciidoctor performance is less than expected, so using a Proxy to cover the length issue for String may be a better way with your (@hmdne) previous approach in #2358. Basically a mutable String would then be just a Proxy of String. |
janbiedermann
changed the title
2 participants
This demonstrates the concept. All is documented at the end of corelib/string.rb.
It implements at the moment String#<< and String#concat .
This has further implications though, to make it work for real. For example:
JS Strings are always considered frozen, even when boxed, so they are immutable.
::String.new strings are mutable (until frozen).
Some compiler changes will be required to choose the correct one. Compiling to a literal/primitive will result in a frozen String, but that's not always wanted. In other cases it should compile to Opal.String.$new() or so.
Ruby 3.4 is also making some changes concerning frozen Strings.
@hmdne + @elia please check, see if this concept is ok for you. If you are ok with this concept i would then continue on that path.