diff --git a/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java b/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java index 56f8b6d39e4..b0b82730d10 100644 --- a/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java +++ b/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java @@ -115,7 +115,8 @@ private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) { case "substr": return tryFoldStringSubstr(subtree, stringNode, firstArg); case "substring": - return tryFoldStringSubstring(subtree, stringNode, firstArg); + case "slice": + return tryFoldStringSubstringOrSlice(subtree, stringNode, firstArg); case "charAt": return tryFoldStringCharAt(subtree, stringNode, firstArg); case "charCodeAt": @@ -129,7 +130,8 @@ private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) { case "substr": return tryReplaceSubstrWithCharAt(subtree, callTarget, firstArg); case "substring": - return tryReplaceSubstringWithCharAt(subtree, callTarget, firstArg); + case "slice": + return tryReplaceSubstringOrSliceWithCharAt(subtree, callTarget, firstArg); } } return subtree; @@ -558,9 +560,9 @@ private Node tryFoldStringSubstr(Node n, Node stringNode, Node arg1) { } /** - * Try to fold .substring() calls on strings + * Try to fold .substring() or .slice() calls on strings */ - private Node tryFoldStringSubstring(Node n, Node stringNode, Node arg1) { + private Node tryFoldStringSubstringOrSlice(Node n, Node stringNode, Node arg1) { Preconditions.checkArgument(n.isCall()); Preconditions.checkArgument(stringNode.isString()); Preconditions.checkArgument(arg1 != null); @@ -598,10 +600,9 @@ private Node tryFoldStringSubstring(Node n, Node stringNode, Node arg1) { // specify the behavior in some of these cases, but we haven't // done a thorough investigation that it is correctly implemented // in all browsers. - if ((end > stringAsString.length()) || - (start > stringAsString.length()) || - (end < 0) || - (start < 0)) { + if ((end > stringAsString.length()) || (start > stringAsString.length()) + || (start < 0) || (end < 0) + || (start > end)) { return n; } @@ -627,7 +628,7 @@ private Node tryReplaceSubstrWithCharAt(Node n, Node callTarget, Node firstArg) return n; } - private Node tryReplaceSubstringWithCharAt(Node n, Node callTarget, Node firstArg) { + private Node tryReplaceSubstringOrSliceWithCharAt(Node n, Node callTarget, Node firstArg) { if (n.getChildCount() != 3) { return n; } @@ -642,18 +643,19 @@ private Node tryReplaceSubstringWithCharAt(Node n, Node callTarget, Node firstAr } int start = maybeStart.intValue(); int end = maybeEnd.intValue(); - if (Math.abs(start - end) == 1) { - return replaceWithCharAt(n, callTarget, start < end ? firstArg : secondArg); + if (start >= end) { + return n; // Bail out for simplicity + } + if (end - start == 1) { + return replaceWithCharAt(n, callTarget, firstArg); } return n; } private Node replaceWithCharAt(Node n, Node callTarget, Node firstArg) { // TODO(moz): Maybe correct the arity of the function type here. - n.detachChildren(); callTarget.getLastChild().setString("charAt"); - n.addChildToFront(callTarget); - n.addChildToBack(firstArg); + firstArg.getNext().detach(); reportCodeChange(); return n; } diff --git a/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java b/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java index 61bd4a13af9..a2c914416b4 100644 --- a/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java +++ b/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java @@ -152,13 +152,28 @@ public void testFoldStringSubstring() { fold("x = 'abcde'['substring'](1,3)", "x = 'bc'"); fold("x = 'abcde'.substring(2)", "x = 'cde'"); - // we should be leaving negative indexes alone for now + // we should be leaving negative, out-of-bound, and inverted indices alone for now foldSame("x = 'abcde'.substring(-1)"); foldSame("x = 'abcde'.substring(1, -2)"); foldSame("x = 'abcde'.substring(1, 2, 3)"); + foldSame("x = 'abcde'.substring(2, 0)"); foldSame("x = 'a'.substring(0, 2)"); } + public void testFoldStringSlice() { + fold("x = 'abcde'.slice(0,2)", "x = 'ab'"); + fold("x = 'abcde'.slice(1,2)", "x = 'b'"); + fold("x = 'abcde'['slice'](1,3)", "x = 'bc'"); + fold("x = 'abcde'.slice(2)", "x = 'cde'"); + + // we should be leaving negative, out-of-bound, and inverted indices alone for now + foldSame("x = 'abcde'.slice(-1)"); + foldSame("x = 'abcde'.slice(1, -2)"); + foldSame("x = 'abcde'.slice(1, 2, 3)"); + foldSame("x = 'abcde'.slice(2, 0)"); + foldSame("x = 'a'.slice(0, 2)"); + } + public void testFoldStringCharAt() { fold("x = 'abcde'.charAt(0)", "x = 'a'"); fold("x = 'abcde'.charAt(1)", "x = 'b'"); @@ -310,19 +325,28 @@ public void testFoldParseOctalNumbers() { fold("x = parseInt(021, 8)", "x = 15"); } - // TODO(moz): Fold String.prototype.slice() public void testReplaceWithCharAt() { enableTypeCheck(); foldStringTyped("a.substring(0, 1)", "a.charAt(0)"); - foldStringTyped("a.substring(2, 1)", "a.charAt(1)"); foldSameStringTyped("a.substring(1, 2, 3)"); foldSameStringTyped("a.substring(i, i + 1)"); // TODO(moz): Fold cases like these foldSameStringTyped("a.substring()"); foldSameStringTyped("a.substring(1)"); foldSameStringTyped("a.substring(1, 3, 4)"); foldSameStringTyped("a.substring(-1, 3)"); + foldSameStringTyped("a.substring(2, 1)"); foldSameStringTyped("a.substring(3, 1)"); + foldStringTyped("a.slice(0, 1)", "a.charAt(0)"); + foldSameStringTyped("a.slice(1, 2, 3)"); + foldSameStringTyped("a.slice(i, i + 1)"); // TODO(moz): Fold cases like these + foldSameStringTyped("a.slice()"); + foldSameStringTyped("a.slice(1)"); + foldSameStringTyped("a.slice(1, 3, 4)"); + foldSameStringTyped("a.slice(-1, 3)"); + foldSameStringTyped("a.slice(2, 1)"); + foldSameStringTyped("a.slice(3, 1)"); + foldStringTyped("a.substr(0, 1)", "a.charAt(0)"); foldStringTyped("a.substr(2, 1)", "a.charAt(2)"); foldStringTyped("a.substr(-2, 1)", "a.charAt(-2)");