Skip to content

Conversation

@MaxGraey
Copy link
Member

@MaxGraey MaxGraey commented Dec 2, 2020

fix #1563

  • I've read the contributing guidelines

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

dcodeIO commented Dec 2, 2020

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
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
Copy link
Member Author

MaxGraey commented Dec 2, 2020

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