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

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

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 // Utilities for building JS ASTs at runtime.  Contains a builder class
 // and a parser that parses part of the language.
 
 part of js_ast;
 
 
@@ skipping 279 matching lines @@
     return new Template.withExpressionResult(ast);
   }
 
   Template statementTemplateYielding(Node ast) {
     return new Template.withStatementResult(ast);
   }
 
   /// Creates a literal js string from [value].
   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'\"';
         case "\b" : return r"\b";
         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);
     // We don't escape ' under the assumption that the string is wrapped
     // into ". Verify that assumption.

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

Have you considered checking whether switching to ' instead of " gives a shorter
program? Or even doing it per string being escaped?

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

Of course. But I didn't yet go ahead and to the optimization.
(I think per string would be easier).

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

This has an issue :-)
https://code.google.com/p/dart/issues/detail?id=22369

     assert(result.value.codeUnitAt(0) == '"'.codeUnitAt(0));
     return result;
   }
 
   /// Creates a literal js string from [value].
   ///
   /// Note that this function only puts quotes around [value]. It does not do
   /// any escaping, so use only when you can guarantee that [value] does not
   /// contain newlines or backslashes. For escaping the string use
   /// [escapedString].
@@ skipping 1001 matching lines @@
   Catch parseCatch() {
     expectCategory(LPAREN);
     String identifier = lastToken;
     expectCategory(ALPHA);
     expectCategory(RPAREN);
     expectCategory(LBRACE);
     Block body = parseBlock();
     return new Catch(new VariableDeclaration(identifier), body);
   }
 }