String to []rune conversion #406
Conversation
There was a problem hiding this comment.
This looks good. However, can you add a few tests that tests various strings (including Unicode)?
You can add them to testdata/slice.go, or perhaps even better rename testdata/rangestring.* to testdata/string.* and add them there. You can test with go test as usual. The output of these Go files must must match the .txt files with the same name in the testdata/ directory. To test out changes to the test files, you can run them with tinygo run testdata/slice.go for example.
compiler/compiler.go
Outdated
| @@ -2473,6 +2473,8 @@ func (c *Compiler) parseConvert(typeFrom, typeTo types.Type, value llvm.Value, p | |||
| switch elemType.Kind() { | |||
| case types.Byte: | |||
| return c.createRuntimeCall("stringToBytes", []llvm.Value{value}, ""), nil | |||
| case types.Rune: | |||
| return c.createRuntimeCall("stringToRunes", []llvm.Value{value}, ""), nil | |||
| default: | |||
| return llvm.Value{}, c.makeError(pos, "todo: convert from string: "+elemType.String()) | |||
There was a problem hiding this comment.
Both cases allowed by the Go spec are now supported, so it's a bug in the compiler if we get here. You can replace the todo: error with a panic. (Note: getting here would be a bug because when we get to SSA level the code has long been type checked and has already been verified as being valid Go).
There was a problem hiding this comment.
I was really not sure what message to put there, please help me with that.
There was a problem hiding this comment.
What about unexpected type in string to slice conversion? It's not hugely important, but it's useful if it is somewhat descriptive.
There was a problem hiding this comment.
Sounds good, I've made the suggested changes. Can you take a look? @aykevl
| n++ | ||
| } | ||
| var r = make([]rune, n) | ||
| n = 0 |
There was a problem hiding this comment.
If you make the change I suggest above, you can remove this line entirely.
| } | ||
| var r = make([]rune, n) | ||
| n = 0 | ||
| for _, e := range s { |
There was a problem hiding this comment.
Seems like you could replace the range here with:
for i := 0; i < n; i++ {
r[i] = s[i]
}
There was a problem hiding this comment.
Unfortunately that doesn't work, indexing into a string returns a byte, not a rune.
|
Thanks for the making some changes to this PR @scriptonist I also made a few suggestions. Can you please add a test that uses a unicode string? Any special characters, such as this example from the golang.org web site, would be sufficient: Thank you. |
|
@deadprogram there is already a test included in this PR and it is actually more extensive than testing a few Chinese (?) characters because it tests various UTF-8 byte lengths (1, 2, 3, and 4 byte characters). |
|
@deadprogram @aykevl Should I add something here, It looked like a NOP for me from the comments. |
|
Thank you for this addition! I have merged the PR in e9f6e51 but modified the commit message to match the style of other commit messages (we follow Go commit message conventions). Sorry it took so long, I got really busy the last few days. |
This PR is aimed at Solving/Fixing #69, I consider this change to be aimed at getting some initial feedback. I'm new to the project and compilers in general. So as the
CONTRIBUTING.mdsuggests, please be kind 😅This change is heavily inspired/copied from the go source.