Bug 150812 - [ES6] Add support for Symbol.replace.
Summary: [ES6] Add support for Symbol.replace.
Status: RESOLVED DUPLICATE of bug 156562
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on: 151359
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-02 13:09 PST by Keith Miller
Modified: 2016-06-06 20:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (48.24 KB, patch)
2015-11-02 13:34 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (48.31 KB, patch)
2015-11-02 13:46 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (786.09 KB, application/zip)
2015-11-02 14:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (777.72 KB, application/zip)
2015-11-02 14:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-mavericks (791.15 KB, application/zip)
2015-11-02 14:54 PST, Build Bot
no flags Details
Patch (81.32 KB, patch)
2015-11-16 16:20 PST, Keith Miller
ggaren: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2015-11-02 13:09:49 PST
[ES6] Add support for Symbol.replace.
Comment 1 Keith Miller 2015-11-02 13:34:42 PST
Created attachment 264619 [details]
Patch
Comment 2 Keith Miller 2015-11-02 13:46:53 PST
Created attachment 264623 [details]
Patch
Comment 3 Geoffrey Garen 2015-11-02 13:59:39 PST
Comment on attachment 264623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264623&action=review

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        In a future patch we need to update the code to reflectthe ES6 spec.

reflect the
Comment 4 Build Bot 2015-11-02 14:42:03 PST
Comment on attachment 264623 [details]
Patch

Attachment 264623 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/373567

New failing tests:
fast/profiler/nested-start-and-stop-profiler.html
Comment 5 Build Bot 2015-11-02 14:42:05 PST
Created attachment 264629 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 6 Build Bot 2015-11-02 14:48:06 PST
Comment on attachment 264623 [details]
Patch

Attachment 264623 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/373585

New failing tests:
fast/profiler/nested-start-and-stop-profiler.html
Comment 7 Build Bot 2015-11-02 14:48:09 PST
Created attachment 264630 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Build Bot 2015-11-02 14:54:12 PST
Comment on attachment 264623 [details]
Patch

Attachment 264623 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/373626

New failing tests:
storage/indexeddb/modern/idbobjectstore-get-failures.html
fast/profiler/nested-start-and-stop-profiler.html
Comment 9 Build Bot 2015-11-02 14:54:15 PST
Created attachment 264632 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 10 Keith Miller 2015-11-02 15:27:41 PST
Hmm, rebasing appears to have broken things. Looking...
Comment 11 Keith Miller 2015-11-16 16:20:53 PST
Created attachment 265636 [details]
Patch
Comment 12 WebKit Commit Bot 2015-11-16 16:22:52 PST
Attachment 265636 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4052:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4054:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Keith Miller 2015-11-16 16:26:07 PST
Comment on attachment 265636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265636&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4054
> +            }

I was unsure what do do here. The NEXT/LAST_OPCODE macros use continue in them so they cannot be wrapped in a do-while loop yet they are multiline so they need braces. There might be a cleaner way to do this but I can't think of it :/
Comment 14 Geoffrey Garen 2015-11-16 17:36:36 PST
Comment on attachment 265636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265636&action=review

The new feature code looks good to me. I don't get the fix to the pre-existing profiler bug in the DFG though.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4054
>> +            }
> 
> I was unsure what do do here. The NEXT/LAST_OPCODE macros use continue in them so they cannot be wrapped in a do-while loop yet they are multiline so they need braces. There might be a cleaner way to do this but I can't think of it :/

I don't get this :(. The important question is whether we expect to execute the ret that follows the call. Is there really a case where A calls B and inlines B, and yet B will execute a ret?

Also, it would be nice to use something more descriptive than a bool to say what happened in handleCall. I'm still not entirely clear one what we're trying to find out from handleCall, though.
Comment 15 Geoffrey Garen 2015-11-17 10:55:57 PST
Comment on attachment 265636 [details]
Patch

Here's what we want:

(1) handleCall should return an enum that tells you whether the call is terminal, which is just a fancy word for a tail call at the level of generated code -- i.e., a tail call from a non-inlined caller or an inlined caller in tail position.

(2) Clients of handleCall that might issue a terminal call (which means op_tail_call and op_tail_call_varargs) should check for terminal, and if terminal, stop all bytecode parsing. This prevents stray bytecodes that are dead from entering the graph, which would trigger assertions and other weirdness.

(3) ByteCodeParser::parseBlock should no longer be permissive in its assertion, at the top, that we never parse past a terminal. 

(4) ByteCodeParser::parseBlock for op_ret should no longer special-case the condition forbidden by (3), since it won't happen anymore.

These changes are complex enough that they should go into a separate patch.
Comment 16 Joseph Pecoraro 2016-06-06 20:27:09 PDT
Symbol.replace support now exists.

I'm guessing it came in as part of:
[ES] Implement RegExp.prototype.@@replace and use it for String.prototype.replace
https://bugs.webkit.org/show_bug.cgi?id=156562
https://trac.webkit.org/changeset/200117

I'm going to close this bug as a dup.

*** This bug has been marked as a duplicate of bug 156562 ***