Fixes #5690: Add trimStart and trimEnd by sharmasuraj0123 · Pull Request #5693 · chakra-core/ChakraCore (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation8 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

{
name: "trimLeft is same function as trimStart",
body: function () {
// NOTE: See comments in test/UnitTestFramework/UnitTestFramework.js for more info about what assertions you can use

Choose a reason for hiding this comment

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

nitpick: please remove these comments that came from the UnitTestFramework template

];

// The test runner will pass "-args summary -endargs" to ch, so that "summary" appears as argument [0[] to the script,
// and the following line will then ensure that the test will only display summary output (i.e. "PASS").

Choose a reason for hiding this comment

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

nitpick: please remove these comments that came from the UnitTestFramework template

assert.areEqual(String.prototype.trimRight, String.prototype.trimEnd, "Both trimRight and trimEnd should point to the same function");
}
}
];

Choose a reason for hiding this comment

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

Please add the following test cases to match other browsers' behavior:

assert.areEqual(String.prototype.trimStart.name, 'trimStart') assert.areEqual(String.prototype.trimEnd.name, 'trimEnd') assert.areEqual(String.prototype.trimLeft.name, 'trimStart') assert.areEqual(String.prototype.trimRight.name, 'trimEnd')

@@ -3994,9 +4000,15 @@ namespace Js
/* No inlining String_EndsWith */ library->AddFunctionToLibraryObject(stringPrototype, PropertyIds::endsWith, &JavascriptString::EntryInfo::EndsWith, 1);
/* No inlining String_Includes */ library->AddFunctionToLibraryObject(stringPrototype, PropertyIds::includes, &JavascriptString::EntryInfo::Includes, 1);
builtinFuncs[BuiltinFunction::JavascriptString_TrimLeft] = library->AddFunctionToLibraryObject(stringPrototype, PropertyIds::trimLeft, &JavascriptString::EntryInfo::TrimLeft, 0);
//builtinFuncs[BuiltinFunction::JavascriptString_TrimStart] = library->AddFunctionToLibraryObject(stringPrototype, PropertyIds::trimStart, &JavascriptString::EntryInfo::TrimStart, 0);

Choose a reason for hiding this comment

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

please remove this commented-out code (I think you already have this change pending)

Choose a reason for hiding this comment

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

Please remove these extra blank lines (I think you already have this change pending)

@@ -244,7 +244,9 @@ namespace Js
static FunctionInfo ToUpperCase;
static FunctionInfo Trim;
static FunctionInfo TrimLeft;
static FunctionInfo TrimStart;
static FunctionInfo TrimRight;

Choose a reason for hiding this comment

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

Any reason why we are keeping a lot of the old TrimLeft/TrimRight references around? I feel like we should be able to effectively change the names entirely and then arbitrarily tack on the start/end entrypoints as trimStart/trimEnd in InitializeStringPrototype.

Choose a reason for hiding this comment

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

trimLeft/trimRight were never included in a spec, but every browser implements them (and pass all these test cases), so we have to keep them. See also the test cases (with eshost output) in #5690

Choose a reason for hiding this comment

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

As for possibly redundant objects -- we will follow up with a clean-up commit

sharmasuraj0123 changed the title~~Add trimStart and trimEnd~~ Fixes #5690: Add trimStart and trimEnd

chakrabot pushed a commit that referenced this pull request

Merge pull request #5693 from sharmasuraj0123:trimStart_trimEnd

Fixes #5690

Labels

Bytecode-Update

This PR updates bytecode and will cause merge conflicts with other PRs with this label