Skip to content
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

Add trim/trimStart/trimEnd to string prototype #30

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

lennartbuit
Copy link
Contributor

@lennartbuit lennartbuit commented Jun 24, 2019

Hey there, I saw your JSConf talk about writing a toy JS engine, and I wanted to contribute. I am not very experienced in Rust, so bear with me :)!


I implemented trim/trimEnd/trimStart to get my toes wet, which are part of #13.

There is a subtle difference in what Rust regards whitespace, and what is considered whitespace in the ECMAScript language. From what I could gather in both languages, it appears that Rust does consider U+0085 (next line) as whitespace but not U+FEFF (zero width non-breaking space), and vice versa for ECMAScript.

I wrote a bit of JavaScript to compare to the v8 impl, all console logs should print true[1]:

function testTrim(ws_char) {
  let str = new String(ws_char + 'abc' + ws_char);
  let trimmed = str.trim();
  console.log(trimmed.length == 3);
}

// Explicit whitespace
testTrim('\u0009');
testTrim('\u000B');
testTrim('\u000C');
testTrim('\u0020');
testTrim('\u00A0');
testTrim('\uFEFF');
// Space sep whitespace
testTrim('\u0020');
testTrim('\u00A0');
testTrim('\u1680');
testTrim('\u2000');
testTrim('\u2001');
testTrim('\u2002');
testTrim('\u2003');
testTrim('\u2004');
testTrim('\u2005');
testTrim('\u2006');
testTrim('\u2007');
testTrim('\u2008');
testTrim('\u2009');
testTrim('\u200A');
testTrim('\u202F');
testTrim('\u205F');
testTrim('\u3000');
// Line terminators
testTrim('\u000A');
testTrim('\u000D');
testTrim('\u2028');
testTrim('\u2029');

Feedback more than welcome!

[1]: Not very pretty JavaScript, but it works in boa :). Implementing Array.prototype.map was too much work for an initial contribution ;)

@jasonwilliams
Copy link
Member

This is awesome work @lennartbuit!
thanks for the contribution, will take a look.

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great, would be good to setup some testing harness for these changes

@jasonwilliams jasonwilliams merged commit 5cfa527 into boa-dev:master Jun 28, 2019
@lennartbuit
Copy link
Contributor Author

lennartbuit commented Jun 28, 2019

I agree, I did take a peek at the test262 harness and/or providing rust based unit tests, but didn't know how to. So thats why I resided to the javascript above.

I am hopeful I can contribute some more this weekend as long as you are willing to tolerate my clear lack of experience in Rust :). Any good 'one Sunday' sized suggestions? Otherwise I'll just fill in another blank of the string prototype :)!

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.

2 participants