-
Notifications
You must be signed in to change notification settings - Fork 112
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
optimisation - remove empty chunks when overwriting #113
Conversation
Looks like this broke the decaffeinate build: decaffeinate/decaffeinate#581. I don't have time to take a look right now, but I can come up with a reduced test case (or maybe @eventualbuddha can) at some point if it's hard to figure out. |
Ah, whoops, sorry – have unpublished 0.17.1 and republished as 0.18. I also don't have time to investigate right now, but at least this way it won't break anyone else's stuff |
Thanks @Rich-Harris! |
Fixes Rich-Harris#115 This commit adds a new `IntegrityCheckingMagicString` class that subclasses `MagicString` and does an integrity check operation, which is used in test code. This revealed 5 bugs, all of which are also fixed in this commit: * Within `indent`, the code was calling `chunk.split`, then making an attempt at properly updating `byStart` and `byEnd`. But it wasn't properly updating `lastChunk`. Rather than calling `chunk.split`, we can just call `this._splitChunk`, which updates `byStart`, `byEnd`, and `lastChunk` correctly. * `move` could sometimes assign `undefined` as a `next` pointer, which is inconsistent since all other cases use `null`. Not a really big deal, but nice to be consistent. * `overwrite` was attempting to do a linked list removal in a way that wasn't properly updating all state ( Rich-Harris#115 ). I tried changing the code to remove the nodes properly, but the details seemed to get really subtle, especially accounting for moved fragments, and it didn't seem like the complexity was worth it, so I effectively just reverted Rich-Harris#113 to fix the issue. But now with the new integrity checking, it should be a lot easier to get the details right if that's desirable. * `trimStart` and `trimEnd` were attempting to update `byStart` and `byEnd`, but they didn't properly set `byEnd` of the newly-created chunk created by `chunk.trimStart`/`chunk.trimEnd`. Setting the `byEnd` for that new chunk seems to fix things. * `trimEnd` was setting `this.lastChunk = chunk.next`, which makes sense for the first iteration (when working with the very last chunk in the linked list), but doesn't make sense for later operations, since it meant that some chunks just get dropped. We now only run that line on the last chunk.
Fixes Rich-Harris#115 This commit adds a new `IntegrityCheckingMagicString` class that subclasses `MagicString` and does an integrity check operation, which is used in test code. This revealed 5 bugs, all of which are also fixed in this commit: * Within `indent`, the code was calling `chunk.split`, then making an attempt at properly updating `byStart` and `byEnd`. But it wasn't properly updating `lastChunk`. Rather than calling `chunk.split`, we can just call `this._splitChunk`, which updates `byStart`, `byEnd`, and `lastChunk` correctly. * `move` could sometimes assign `undefined` as a `next` pointer, which is inconsistent since all other cases use `null`. Not a really big deal, but nice to be consistent. * `overwrite` was attempting to do a linked list removal in a way that wasn't properly updating all state ( Rich-Harris#115 ). I tried changing the code to remove the nodes properly, but the details seemed to get really subtle, especially accounting for moved chunks, and it didn't seem like the complexity was worth it, so I effectively just reverted Rich-Harris#113 to fix the issue. But now with the new integrity checking, it should be a lot easier to get the details right if that's desirable. * `trimStart` and `trimEnd` were attempting to update `byStart` and `byEnd`, but they didn't properly set `byEnd` of the newly-created chunk created by `chunk.trimStart`/`chunk.trimEnd`. Setting the `byEnd` for that new chunk seems to fix things. * `trimEnd` was setting `this.lastChunk = chunk.next`, which makes sense for the first iteration (when working with the very last chunk in the linked list), but doesn't make sense for later iterations, since it meant that some chunks just get dropped. We now only run that line on the last chunk.
This collapses any obsolete chunks into one following
overwrite
orremove
– i.e. instead of the edited chunk containing the overwrite followed by N empty chunks, it links the edited chunk directly to the next one. Linked lists FTW