-
- Notifications
You must be signed in to change notification settings - Fork 678
Add String#repeat method #67
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
Changes from 12 commits
9e1364f aabb7a6 fd7c2c7 1bc1f6b 3685cc0 d34c36b 534b679 5e7dbcf 749ae3e 343eec0 938ad8e 837c6f7 946df47 9853c0f 192fcd4 a251c98 File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -131,7 +131,9 @@ export class String { | |
| | ||
| @operator(">") | ||
| private static __gt(left: String, right: String): bool { | ||
| if (left === null || right === null) return false; | ||
| if (left === right || left === null || right === null) { | ||
| return false; | ||
| } | ||
| | ||
| var leftLength = left.length; | ||
| var rightLength = right.length; | ||
| | @@ -149,8 +151,9 @@ export class String { | |
| | ||
| @operator(">=") | ||
| private static __gte(left: String, right: String): bool { | ||
| if (left === right) return true; | ||
| if (left === null) return right === null; | ||
| else if (right === null) return false; | ||
| if (right === null) return false; | ||
| | ||
| var leftLength = left.length; | ||
| var rightLength = right.length; | ||
| | @@ -168,7 +171,9 @@ export class String { | |
| | ||
| @operator("<") | ||
| private static __lt(left: String, right: String): bool { | ||
| if (left === null || right === null) return false; | ||
| if (left === right || left === null || right === null) { | ||
| return false; | ||
| } | ||
| | ||
| var leftLength = left.length; | ||
| var rightLength = right.length; | ||
| | @@ -186,6 +191,7 @@ export class String { | |
| | ||
| @operator("<=") | ||
| private static __lte(left: String, right: String): bool { | ||
| if (left === right) return true; | ||
| if (left === null) return right === null; | ||
| else if (right === null) return false; | ||
| ||
| | ||
| | @@ -377,6 +383,40 @@ export class String { | |
| ); | ||
| return out; | ||
| } | ||
| | ||
| repeat(count: i32 = 0): String { | ||
| assert(this !== null); | ||
| var length = this.length; | ||
| | ||
| // Most browsers can't handle strings 1 << 28 chars or longer | ||
| if (count < 0 || length * count > (1 << 28)) { | ||
| throw new RangeError("Invalid count value"); | ||
| } | ||
| | ||
| if (count === 0 || !length) return EMPTY; | ||
| if (count === 1) return this; | ||
| | ||
| var result = allocate(length * count); | ||
| Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this missing the leading 32-bit length? Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I use this allocation method which already include HEADER size during allocation if I right understand your comment =) Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yeah, makes sense then. | ||
| var strLen = length << 1; | ||
| | ||
| /* | ||
| * TODO possible improvments: reuse existing result for exponentially concats like: | ||
| * 'a' + 'a' => 'aa' + 'aa' => 'aaaa' + 'aaaa' etc | ||
| */ | ||
| for (let offset = 0, len = strLen * count; offset < len; offset += strLen) { | ||
| move_memory( | ||
| Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess copy size could also increase. Like, repeating "a" four times would first create "aa" and then copy "aa" to "aaaa". Wdyt? Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it, and move_memory may better optimize for copy large world chunks, but this increase codebase and but benefits may be not so huge. Need benchmark. For now I can add TODO comment for future if don't mind | ||
| changetype<usize>(result) + HEADER_SIZE + offset, | ||
| changetype<usize>(this) + HEADER_SIZE, | ||
| strLen | ||
| ); | ||
| } | ||
| | ||
| return result; | ||
| } | ||
| | ||
| toString(): String { | ||
| return this; | ||
| } | ||
| } | ||
| | ||
| export function parseInt(str: String, radix: i32 = 0): f64 { | ||
| | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In stdlib, I'm trying to avoid such blocks. Doesn't look as nice, but the parser ultimately can skip producing one BlockStatement, slightly reducing memory footprint. Not sure if that's overoptimization already, but I had the feeling it might be okish to do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I should refactor this to 3-timed
ifinstead one block?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just
if (left === right || left === null || right === null) return false;, so it results in the following AST:instead of
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this. But I can't see any changes even in
untouchedwast except assert's column/line numbers. Is it normal? It seems this overoptimization)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does compile to the same thing, but the parser has to allocate one less AST node. My though was that, since stdlib is always included in compilation and fully parsed (unlike other compilers that might link it in), that it would accumulate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, got it