Move JSSyntaxRegExp to core as a private member. This removes the last refrences to dart:coreimpl.

Move JSSyntaxRegExp to core as a private member. This removes the last refrences to dart:coreimpl.

After this cleanup, RegExp no longer have a const constructor. Use 'new
RegExp(...)' from now on.

BUG=

Committed: https://code.google.com/p/dart/source/detail?r=14838

[Anders Johnsen, 2012-11-12 12:16:59]

Peter, can you look at the Dart2JS changes?

Konstantin, can you look at the Editor changes?

Mads, can you look at the VM changes?

Thank you, all!

[Mads Ager (google), 2012-11-12 12:28:10]

VM changes look good to me.

https://codereview.chromium.org/11365196/diff/2001/runtime/lib/regexp_patch.dart
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/11365196/diff/2001/runtime/lib/regexp_patch.d...
runtime/lib/regexp_patch.dart:9: => new _JSSyntaxRegExp(pattern,
I don't like the arrow when the body takes up this much space. Could you use
braces instead?

[Anders Johnsen, 2012-11-12 12:30:31]

https://codereview.chromium.org/11365196/diff/2001/runtime/lib/regexp_patch.dart
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/11365196/diff/2001/runtime/lib/regexp_patch.d...
runtime/lib/regexp_patch.dart:9: => new _JSSyntaxRegExp(pattern,
On 2012/11/12 12:28:10, Mads Ager wrote:
> I don't like the arrow when the body takes up this much space. Could you use
> braces instead?

Sure, done!

[ahe, 2012-11-12 12:55:23]

I would be awesome if you could separate the changes from 'const RegExp' -> 'new
RegExp' from the library changes.

In case you have to revert, that avoids a lot of fluctuation.  Also, it makes it
easier to reason about the library change.

[floitsch, 2012-11-12 16:31:56]

LGTM.
Adding Alan and Bob as CC since most of the const->new changes are in their
code.

https://codereview.chromium.org/11365196/diff/6001/pkg/intl/lib/date_format.dart
File pkg/intl/lib/date_format.dart (right):

https://codereview.chromium.org/11365196/diff/6001/pkg/intl/lib/date_format.d...
pkg/intl/lib/date_format.dart:448: static var _matchers = [
Not your code, but I would type the variable. Especially with a lazy
initializer.

https://codereview.chromium.org/11365196/diff/6001/runtime/lib/regexp_patch.dart
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/lib/regexp_patch.d...
runtime/lib/regexp_patch.dart:66: class _JSSyntaxRegExp extends RegExp {
why not just "implements"?

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/bootstrap_nativ...
File runtime/vm/bootstrap_natives.cc (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/bootstrap_nativ...
runtime/vm/bootstrap_natives.cc:63: library = Library::CollectionLibrary();
two "Library::CollectionLibrary loads.

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6032: RawLibrary* Library::CollectionLibrary() {
2 collection-libraries.

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/snapshot_test.c...
runtime/vm/snapshot_test.cc:871: lib = Library::IsolateLibrary();
two Isolate-libraries.

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:378: // take advantage
of this as core import js_helper and see Dynamic this
"imports" "sees" "[dynamic]".

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:423: // The core library
were loaded and patched before jsHelperLibrary was
was

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/core/sort.dart
File sdk/lib/core/sort.dart (right):

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/core/sort.dart#new...
sdk/lib/core/sort.dart:5: // TODO(ajohnsen): Remove once dart2js intercepters
can work without.
without it.

[Anders Johnsen, 2012-11-12 16:58:38]

Thank you!

https://codereview.chromium.org/11365196/diff/6001/pkg/intl/lib/date_format.dart
File pkg/intl/lib/date_format.dart (right):

https://codereview.chromium.org/11365196/diff/6001/pkg/intl/lib/date_format.d...
pkg/intl/lib/date_format.dart:448: static var _matchers = [
On 2012/11/12 16:31:56, floitsch wrote:
> Not your code, but I would type the variable. Especially with a lazy
> initializer.

Will address in https://codereview.chromium.org/11410033/.

https://codereview.chromium.org/11365196/diff/6001/runtime/lib/regexp_patch.dart
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/lib/regexp_patch.d...
runtime/lib/regexp_patch.dart:66: class _JSSyntaxRegExp extends RegExp {
On 2012/11/12 16:31:56, floitsch wrote:
> why not just "implements"?

Done.

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/bootstrap_nativ...
File runtime/vm/bootstrap_natives.cc (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/bootstrap_nativ...
runtime/vm/bootstrap_natives.cc:63: library = Library::CollectionLibrary();
On 2012/11/12 16:31:56, floitsch wrote:
> two "Library::CollectionLibrary loads.

Bad merge.

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6032: RawLibrary* Library::CollectionLibrary() {
On 2012/11/12 16:31:56, floitsch wrote:
> 2 collection-libraries.


Bad merge.

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

https://codereview.chromium.org/11365196/diff/6001/runtime/vm/snapshot_test.c...
runtime/vm/snapshot_test.cc:871: lib = Library::IsolateLibrary();
On 2012/11/12 16:31:56, floitsch wrote:
> two Isolate-libraries.

Bad merge.

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:378: // take advantage
of this as core import js_helper and see Dynamic this
On 2012/11/12 16:31:56, floitsch wrote:
> "imports" "sees" "[dynamic]".

Done.

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:423: // The core library
were loaded and patched before jsHelperLibrary was
On 2012/11/12 16:31:56, floitsch wrote:
> was

Done.

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/core/sort.dart
File sdk/lib/core/sort.dart (right):

https://codereview.chromium.org/11365196/diff/6001/sdk/lib/core/sort.dart#new...
sdk/lib/core/sort.dart:5: // TODO(ajohnsen): Remove once dart2js intercepters
can work without.
On 2012/11/12 16:31:56, floitsch wrote:
> without it.

Done.

[Anders Johnsen, 2012-11-12 17:09:16]

Alan, Bob, check out https://codereview.chromium.org/11410033/ for the changes.
I've isolated the const->new changes over here.

[scheglov, 2012-11-12 23:09:22]

lgtm

[Ivan Posva, 2012-11-13 18:26:27]

Does not LGTM, there are too many inconsistencies in the VM related code. Please
make sure to have a VM reviewer when doing VM related changes!

-Ivan

https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.gypi
File runtime/lib/lib_sources.gypi (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.g...
runtime/lib/lib_sources.gypi:31: 'math.dart',
Why was this moved here? The functions in this file should definitely not end up
in dart:core.

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/bootstrap_nati...
File runtime/vm/bootstrap_natives.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/bootstrap_nati...
runtime/vm/bootstrap_natives.cc:63: library = Library::CollectionLibrary();
Does the collection library have any native code?

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/intrinsifier.c...
runtime/vm/intrinsifier.cc:38: String::Concat(test_str,
String::Handle(scalarlist_lib.private_key()));
How does this differ from the code above?

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6066: RawLibrary* Library::CollectionLibrary() {
Unless I am severely mistaken this code cannot compile.

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:9016:
Class::Handle(LookupCoreClass(immutable_map_class_name));
I do not see the immutable map class moving from coreimpl to core in this
change, how did that work?

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/snapshot_test....
runtime/vm/snapshot_test.cc:875: // Check isolate lib.
?

[Anders Johnsen, 2012-11-13 18:40:26]

As the log will show, Ager did review the VM changes, and many of the
inconsistencies you found were actually bad merge/reitveld doing funky stuff.

You do however raise a few very good points, that I really would appreciate some
further feedback on!

- Anders

https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.gypi
File runtime/lib/lib_sources.gypi (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.g...
runtime/lib/lib_sources.gypi:31: 'math.dart',
On 2012/11/13 18:26:28, Ivan Posva wrote:
> Why was this moved here? The functions in this file should definitely not end
up
> in dart:core.

It lived in coreimpl before, so that's why it was moved here. Let me know where
you would like this to end.

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/bootstrap_nati...
File runtime/vm/bootstrap_natives.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/bootstrap_nati...
runtime/vm/bootstrap_natives.cc:63: library = Library::CollectionLibrary();
On 2012/11/13 18:26:28, Ivan Posva wrote:
> Does the collection library have any native code?

This is actually not the change, it's a bad merge. But your point is valid,
collection does currently not have any native code - yet. We are (were) not sure
what we will end up with, so it ended here in the beginning. However, in the
direction it's going now, we'll probably not end up having any native code here,
thus at least a TODO is in order!

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/intrinsifier.c...
runtime/vm/intrinsifier.cc:38: String::Concat(test_str,
String::Handle(scalarlist_lib.private_key()));
On 2012/11/13 18:26:28, Ivan Posva wrote:
> How does this differ from the code above?

Bad merge.

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6066: RawLibrary* Library::CollectionLibrary() {
On 2012/11/13 18:26:28, Ivan Posva wrote:
> Unless I am severely mistaken this code cannot compile.

This is not actually what happened, as I wrote in an earlier comment. It's a bad
merge. There is only one of them.

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:9016:
Class::Handle(LookupCoreClass(immutable_map_class_name));
On 2012/11/13 18:26:28, Ivan Posva wrote:
> I do not see the immutable map class moving from coreimpl to core in this
> change, how did that work?

Agreed, and this is something that I didn't detect earlier on. This is 100% my
error to not have detected this correctly in an earlier commit (a few weeks
ago). Nonetheless, status quo is that coreimpl doesn't exist anymore.

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/vm/snapshot_test....
runtime/vm/snapshot_test.cc:875: // Check isolate lib.
On 2012/11/13 18:26:28, Ivan Posva wrote:
> ?

Bad merge.

[Mads Ager (google), 2012-11-14 10:32:31]

https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.gypi
File runtime/lib/lib_sources.gypi (right):

https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.g...
runtime/lib/lib_sources.gypi:31: 'math.dart',
On 2012/11/13 18:40:26, ajohnsen wrote:
> On 2012/11/13 18:26:28, Ivan Posva wrote:
> > Why was this moved here? The functions in this file should definitely not
end
> up
> > in dart:core.
> 
> It lived in coreimpl before, so that's why it was moved here. Let me know
where
> you would like this to end.

The problem is not the move. The problem is that MathNatives now becomes a part
of the VM core library. It shouldn't be. We need to make MathNatives a private
class in dart:core so we do not extend core. I should have caught this in the
review. Sorry.

[Ivan Posva, 2012-11-15 09:14:46]

On 2012/11/14 10:32:31, Mads Ager wrote:
>
https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.gypi
> File runtime/lib/lib_sources.gypi (right):
> 
>
https://codereview.chromium.org/11365196/diff/11067/runtime/lib/lib_sources.g...
> runtime/lib/lib_sources.gypi:31: 'math.dart',
> On 2012/11/13 18:40:26, ajohnsen wrote:
> > On 2012/11/13 18:26:28, Ivan Posva wrote:
> > > Why was this moved here? The functions in this file should definitely not
> end
> > up
> > > in dart:core.
> > 
> > It lived in coreimpl before, so that's why it was moved here. Let me know
> where
> > you would like this to end.
> 
> The problem is not the move. The problem is that MathNatives now becomes a
part
> of the VM core library. It shouldn't be. We need to make MathNatives a private
> class in dart:core so we do not extend core. I should have caught this in the
> review. Sorry.

Actually since MathNatives is only used from within dart:math it should move
there instead. I do have a change locally which deals with this and other
related cleanup.

-Ivan