Make RegExp's constructor non-const.

Make RegExp's constructor non-const.


BUG=

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

[Anders Johnsen, 2012-11-12 13:28:16]

I'll send an email out as soon as this is 'LGTM'.

[ahe, 2012-11-12 13:35:03]

Not sure about the dart2js changes.

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js/printer.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js/printer.dart:49: final
identifierRegexp = new RegExp(r'^[a-zA-Z_0-9$]');
I think this should be a static final field.

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/coreimpl_patch.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/coreimpl_patch.dart:15: patch
JSSyntaxRegExp(String pattern,
I'm not sure this will work here as you aren't updating the core library.

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/string_helper.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:95: new
JSSyntaxRegExp(r'[-[\]{}()*+?.,\\^$|#\s]');
Top-level final field?

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/native_handler.dart:214: RegExp
nativeRedirectionRegExp = new RegExp(r'^[a-zA-Z][a-zA-Z_$0-9]*$');
Top-level final field?

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/variable_allocator.dart
(right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/variable_allocator.dart:553:
RegExp regexp = new RegExp('t[0-9]+');
Top-level final field?

[Anders Johnsen, 2012-11-12 13:45:38]

PTAL

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js/printer.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js/printer.dart:49: final
identifierRegexp = new RegExp(r'^[a-zA-Z_0-9$]');
On 2012/11/12 13:35:03, ahe wrote:
> I think this should be a static final field.

Done.

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/coreimpl_patch.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/coreimpl_patch.dart:15: patch
JSSyntaxRegExp(String pattern,
On 2012/11/12 13:35:03, ahe wrote:
> I'm not sure this will work here as you aren't updating the core library.

Not part of this CL :)

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/string_helper.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/string_helper.dart:95: new
JSSyntaxRegExp(r'[-[\]{}()*+?.,\\^$|#\s]');
On 2012/11/12 13:35:03, ahe wrote:
> Top-level final field?

Done.

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/native_handler.dart:214: RegExp
nativeRedirectionRegExp = new RegExp(r'^[a-zA-Z][a-zA-Z_$0-9]*$');
On 2012/11/12 13:35:03, ahe wrote:
> Top-level final field?

Done.

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/variable_allocator.dart
(right):

https://codereview.chromium.org/11410033/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/variable_allocator.dart:553:
RegExp regexp = new RegExp('t[0-9]+');
On 2012/11/12 13:35:03, ahe wrote:
> Top-level final field?

Done, +static.

[ahe, 2012-11-12 13:51:28]

Changes to sdk/lib/_internal/compiler, LGTM! Thanks for doing this.

[ahe, 2012-11-12 13:53:49]

Changes to test/ also LGTM!

https://codereview.chromium.org/11410033/diff/2002/sdk/lib/uri/uri.dart
File sdk/lib/uri/uri.dart (right):

https://codereview.chromium.org/11410033/diff/2002/sdk/lib/uri/uri.dart#newcode1
sdk/lib/uri/uri.dart:1: // Copyright (c) 2012, the Dart project authors.  Please
see the AUTHORS file
This file, LGTM

[Alan Knight, 2012-11-12 18:31:05]

Overall seems reasonable. Do we have any idea what the performance impact of
that might be, as we now have to construct a new RegExp on each call? Is it
worthwhile moving the RegExp's into static variables which can then be lazily
initialized and re-used?

https://codereview.chromium.org/11410033/diff/11002/pkg/unittest/lib/interact...
File pkg/unittest/lib/interactive_html_config.dart (right):

https://codereview.chromium.org/11410033/diff/11002/pkg/unittest/lib/interact...
pkg/unittest/lib/interactive_html_config.dart:261: static const _notAlphaNumeric
= new RegExp('[^a-z0-9A-Z]');
Is this going to work? If the variable is declared static const, then doesn't
the object have to be const as well?

[Bob Nystrom, 2012-11-12 18:55:05]

Reviewed:

pkg/args
pkg/http
samples/markdown
utils/apidoc
utils/pub
utils/tests/pub

Those parts LGTM once my comments are addressed.

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/block_pa...
File samples/markdown/block_parser.dart (right):

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/block_pa...
samples/markdown/block_parser.dart:6: const _RE_EMPTY = new RegExp(r'^([
\t]*)$');
These will all need to be "final" now, right?

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/html_ren...
File samples/markdown/html_renderer.dart (right):

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/html_ren...
samples/markdown/html_renderer.dart:9: static const _BLOCK_TAGS = new RegExp(
"const" -> "final"

https://codereview.chromium.org/11410033/diff/11002/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/11410033/diff/11002/utils/pub/git_source.dart...
utils/pub/git_source.dart:155: final URL_LIKE = new RegExp(r'^[a-z]+\:');
"final" -> "var". We don't bother to use "final" for locals in pub.

https://codereview.chromium.org/11410033/diff/11002/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11410033/diff/11002/utils/pub/io.dart#newcode24
utils/pub/io.dart:24: final Pattern NEWLINE_PATTERN = new RegExp("\r\n?|\n\r?");
Can you remove the type annotation here while you're at it?

https://codereview.chromium.org/11410033/diff/11002/utils/pub/io.dart#newcode385
utils/pub/io.dart:385: final ABSOLUTE = new RegExp(r'^(\\\\|[a-zA-Z]:[/\\])');
"final" -> "var"

[Anders Johnsen, 2012-11-13 06:42:32]

Thank you all for the reviews! :)

https://codereview.chromium.org/11410033/diff/11002/pkg/unittest/lib/interact...
File pkg/unittest/lib/interactive_html_config.dart (right):

https://codereview.chromium.org/11410033/diff/11002/pkg/unittest/lib/interact...
pkg/unittest/lib/interactive_html_config.dart:261: static const _notAlphaNumeric
= new RegExp('[^a-z0-9A-Z]');
On 2012/11/12 18:31:05, Alan Knight wrote:
> Is this going to work? If the variable is declared static const, then doesn't
> the object have to be const as well?

You are right, it looks like this is not run by any tests? Fixed.

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/block_pa...
File samples/markdown/block_parser.dart (right):

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/block_pa...
samples/markdown/block_parser.dart:6: const _RE_EMPTY = new RegExp(r'^([
\t]*)$');
On 2012/11/12 18:55:05, Bob Nystrom wrote:
> These will all need to be "final" now, right?

Yep. Again, looks like we don't test this?

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/html_ren...
File samples/markdown/html_renderer.dart (right):

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/html_ren...
samples/markdown/html_renderer.dart:9: static const _BLOCK_TAGS = new RegExp(
On 2012/11/12 18:55:05, Bob Nystrom wrote:
> "const" -> "final"

Done.

https://codereview.chromium.org/11410033/diff/11002/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/11410033/diff/11002/utils/pub/git_source.dart...
utils/pub/git_source.dart:155: final URL_LIKE = new RegExp(r'^[a-z]+\:');
On 2012/11/12 18:55:05, Bob Nystrom wrote:
> "final" -> "var". We don't bother to use "final" for locals in pub.

Done.

https://codereview.chromium.org/11410033/diff/11002/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11410033/diff/11002/utils/pub/io.dart#newcode24
utils/pub/io.dart:24: final Pattern NEWLINE_PATTERN = new RegExp("\r\n?|\n\r?");
On 2012/11/12 18:55:05, Bob Nystrom wrote:
> Can you remove the type annotation here while you're at it?

Done.

https://codereview.chromium.org/11410033/diff/11002/utils/pub/io.dart#newcode385
utils/pub/io.dart:385: final ABSOLUTE = new RegExp(r'^(\\\\|[a-zA-Z]:[/\\])');
On 2012/11/12 18:55:05, Bob Nystrom wrote:
> "final" -> "var"

Done.

[sra1, 2012-11-13 07:12:08]

Can someone explain why we are making this change?

[Bob Nystrom, 2012-11-13 19:14:13]

https://codereview.chromium.org/11410033/diff/11002/pkg/unittest/lib/interact...
File pkg/unittest/lib/interactive_html_config.dart (right):

https://codereview.chromium.org/11410033/diff/11002/pkg/unittest/lib/interact...
pkg/unittest/lib/interactive_html_config.dart:261: static const _notAlphaNumeric
= new RegExp('[^a-z0-9A-Z]');
On 2012/11/13 06:42:32, ajohnsen wrote:
> On 2012/11/12 18:31:05, Alan Knight wrote:
> > Is this going to work? If the variable is declared static const, then
doesn't
> > the object have to be const as well?
> 
> You are right, it looks like this is not run by any tests? Fixed.

unittest does have test coverage now, but this probably doesn't get caught by it
since it's a specific test running configuration.

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/block_pa...
File samples/markdown/block_parser.dart (right):

https://codereview.chromium.org/11410033/diff/11002/samples/markdown/block_pa...
samples/markdown/block_parser.dart:6: const _RE_EMPTY = new RegExp(r'^([
\t]*)$');
On 2012/11/13 06:42:32, ajohnsen wrote:
> On 2012/11/12 18:55:05, Bob Nystrom wrote:
> > These will all need to be "final" now, right?
> 
> Yep. Again, looks like we don't test this?

Nope. :(

This is basically a copy of the markdown parsing code in dartdoc. It will
probably disappear from here before too long.