Skip to content

fix: Fix wrong reallocations in String#replaceAll + refactoring - #1564

Merged
dcodeIO merged 4 commits into
AssemblyScript:masterfrom
MaxGraey:fix-replaceall
Dec 3, 2020
Merged

fix: Fix wrong reallocations in String#replaceAll + refactoring#1564
dcodeIO merged 4 commits into
AssemblyScript:masterfrom
MaxGraey:fix-replaceall

Conversation

@MaxGraey

@MaxGraey MaxGraey commented Dec 2, 2020

Copy link
Copy Markdown
Member

fix #1563

  • I've read the contributing guidelines

@MaxGraey
MaxGraey requested a review from dcodeIO December 2, 2020 12:47
@dcodeIO

dcodeIO commented Dec 2, 2020

Copy link
Copy Markdown
Member

I find it a little hard to grasp the variable names in this function. Would you be OK with making these a little more self explaining while on it? For example

  • len -> thisLen
  • slen -> searchLen
  • rlen -> replaceLen
  • size -> outSize
  • resLen -> outLen
  • chunk -> ?

@MaxGraey MaxGraey changed the title fix: Fix wrong reallocations in String#replaceAll fix: Fix wrong reallocations in String#replaceAll + refactoring Dec 2, 2020
@ColinEberhardt

Copy link
Copy Markdown
Contributor

Great to see this fixed so quickly, thanks @MaxGraey

Just out of curiosity, what is the development / debug experience for the core AssemblyScript libraries? Is it possible to step through, inspect, or perhaps just log, when trying to resolve these errors?

@MaxGraey

MaxGraey commented Dec 2, 2020

Copy link
Copy Markdown
Member Author

Just out of curiosity, what is the development / debug experience for the core AssemblyScript libraries? Is it possible to step through, inspect, or perhaps just log, when trying to resolve these errors?

wasm pretty hard to debug especially on node.js. I usually use trace and assert buiiltin functions. For very hard cases - just reproduce same code on ts/js and debug as usual)

@dcodeIO
dcodeIO merged commit 1fcc374 into AssemblyScript:master Dec 3, 2020
@MaxGraey
MaxGraey deleted the fix-replaceall branch December 3, 2020 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

replaceAll doesn't allow replacements that change the string length

3 participants