dart2js: Avoid escaping in strings if it's not necessary.

pkg/compiler/lib/src/js/builder.dart

Index: pkg/compiler/lib/src/js/builder.dart
diff --git a/pkg/compiler/lib/src/js/builder.dart b/pkg/compiler/lib/src/js/builder.dart
index 72b4507081ebca30903142b36121319d01c75640..125a5d1c2d4d6951b0fb7fc4e89b765a1bb913b8 100644
--- a/pkg/compiler/lib/src/js/builder.dart
+++ b/pkg/compiler/lib/src/js/builder.dart
@@ -298,9 +298,32 @@ class JsBuilder {
   LiteralString escapedString(String value) {
    // Start by escaping the backslashes.
     String escaped = value.replaceAll('\\', '\\\\');

[Lasse Reichstein Nielsen, 2015/02/23 13:25:05]

Why not combine the backslash-escaping in the other replaceAllMapped?
 

[floitsch, 2015/02/23 16:56:41]

Done.

+    // Relevant sections of ECMA-262:
+    //
+    //     7.3 Line Terminators
+    //       LineTerminator ::
+    //          <LF>
+    //          <CR>
+    //          <LS>
+    //          <PS>
+    //
+    //     7.8.4 String Literals
+    //       StringLiteral ::
+    //          " DoubleStringCharacters? "
+    //          ' SingleStringCharacters? '
+    //
+    //       DoubleStringCharacters ::
+    //          DoubleStringCharacter DoubleStringCharacters?
+    //
+    //       DoubleStringCharacter ::
+    //          SourceCharacter but not one of " or \ or LineTerminator
+    //          \ EscapeSequence
+    //          LineContinuation
+    //
     // Do not escape unicode characters and ' because they are allowed in the
     // string literal anyway.

[Lasse Reichstein Nielsen, 2015/02/23 13:25:05]

Technically, you don't have to escape \b, \t, \f and \v. They are not line
terminator, so they are allowed in output. It's only for readability, not size,
that they are escaped here.
It's probably still a good idea to spare an extra byte per instance for
readability.

[floitsch, 2015/02/23 16:56:41]

Yes. My initial patch (that wasn't using this function) didn't encode it. But I
think it's cleaner this way too.

[sra1, 2015/02/23 17:47:27]

I think we can have more than one version of this method that can be used
depending on context.
For example, a version that omits '\t' -> r'\t' might be called from the
constant emitter in minified mode.

-    escaped = escaped.replaceAllMapped(new RegExp('\n|"|\b|\t|\v'), (match) {
+    escaped = escaped.replaceAllMapped(
+        new RegExp('\n|"|\b|\t|\v|\u2028|\u2029'), (match) {

[sra1, 2015/02/23 17:47:27]

new RegExp -> static variable.

[floitsch, 2015/02/23 18:16:45]

It reads less nice, but done.
Called "_stringEscapeRegExp".

[Lasse Reichstein Nielsen, 2015/02/24 14:14:21]

How about:
  new RegExp(r'[\n\b\t\v"\u2028\u2029]')
instead? I don't think it's any faster, but it is slightly more idiomatic when
you are switching on single characters.

[floitsch, 2015/02/24 20:49:44]

Done.

       switch (match.group(0)) {
         case "\n" : return r"\n";
         case "\"" : return r'\"';
@@ -308,6 +331,8 @@ class JsBuilder {
         case "\t" : return r"\t";
         case "\f" : return r"\f";
         case "\v" : return r"\v";
+        case "\u2028" : return r"\u2028";
+        case "\u2029" : return r"\u2029";
       }
     });

[Lasse Reichstein Nielsen, 2015/02/23 13:25:05]

Does this have a measurable performance impact? Because it does look slightly
slow creating one-character strings for each match and doing a string-switch
instead of looking at the code-unit directly.

[floitsch, 2015/02/23 16:56:41]

I tried with a string of 500000 \u2029 characters (probably the worst-case
scenario) and didn't see any difference in performance.

     LiteralString result = string(escaped);