Const enum variants no longer backed by functions #507
Conversation
This file contains 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
Rather than having constant enum variants backed by a function call like how container enum variants work, store them as constant `data` values instead. Constant enum variants don't contain variables or data which could vary depending on construction, so this is a pretty straightforward (if minor) performance improvement. The real main change hidden in this changeset is the solving of a sneaky and nefarious bug that's been hidden in here for a while - when storing Byte instances into a Pointer value using the intrinsic `Pointer#store` method, it would actually use a `storel` qbe instruction behind the scenes which would result in _4_ bytes being written to the underlying memory. This had the evil side effect of overwriting the neighboring memory segment's value, most likely to be `0`. This was most apparent in `Array#toString` which I had actually already noticed a while ago but never got around to actually digging in to figure out what was wrong. This solution was the result of many hours' work (unfortunately) so it's no wonder why I punted on it for so long. Oh well, it was fun.
kengorab
commented
Nov 24, 2024
| func _callMalloc(self, sizeVal: Value, localName: String? = None): Result<Value, CompileError> { | ||
| val mem = try self._currentFn.block.buildCall(Callable.Function(self._malloc), [sizeVal], localName) else |e| return qbeError(e) | ||
|
|
||
| // val labelIsZero = self._currentFn.block.addLabel("malloc_is_zero") |
There was a problem hiding this comment.
Can uncomment this if I ever want to introduce this kind of runtime check.
kengorab
commented
Nov 24, 2024
| // // Array#flatMap | ||
| // (() => { | ||
| // val arr = [1, 2, 3, 4] | ||
| // Array#flatMap |
There was a problem hiding this comment.
Pretty nice that this test passes, now that I finally addressed that nefarious bug
|
Closes #506 |
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.
Rather than having constant enum variants backed by a function call like how container enum variants work, store them as constant
datavalues instead. Constant enum variants don't contain variables or data which could vary depending on construction, so this is a pretty straightforward (if minor) performance improvement.The real main change hidden in this changeset is the solving of a sneaky and nefarious bug that's been hidden in here for a while - when storing Byte instances into a Pointer value using the intrinsic
Pointer#storemethod, it would actually use astorelqbe instruction behind the scenes which would result in 4 bytes being written to the underlying memory. This had the evil side effect of overwriting the neighboring memory segment's value, most likely to be0. This was most apparent inArray#toStringwhich I had actually already noticed a while ago but never got around to actually digging in to figure out what was wrong. This solution was the result of many hours' work (unfortunately) so it's no wonder why I punted on it for so long. Oh well, it was fun.