Moving logic for writing source maps from SourceMapBuilder into the Mapping class.

pkg/source_maps/lib/parser.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.
 
 /// Contains the top-level function to parse source maps version 3.
 library source_maps.parser;
 
+import 'dart:collection';
 import 'dart:convert';
 
+import 'builder.dart';
 import 'span.dart';
 import 'src/utils.dart';
 import 'src/vlq.dart';
 
 /// Parses a source map directly from a json string.
 // TODO(sigmund): evaluate whether other maps should have the json parsed, or
 // the string represenation.
 Mapping parse(String jsonMap, {Map<String, Map> otherMaps}) =>
   parseJson(JSON.decode(jsonMap), otherMaps: otherMaps);
 
@@ skipping 14 matching lines @@
         map.containsKey('names')) {
       throw new FormatException('map containing "sections" '
           'cannot contain "mappings", "sources", or "names".');
     }
     return new MultiSectionMapping.fromJson(map['sections'], otherMaps);
   }
   return new SingleMapping.fromJson(map);
 }
 
 
-/// A mapping parsed our of a source map.
+/// A mapping parsed out of a source map.
 abstract class Mapping {
   Span spanFor(int line, int column, {Map<String, SourceFile> files});
 
   Span spanForLocation(Location loc, {Map<String, SourceFile> files}) {
     return spanFor(loc.line, loc.column, files: files);
   }
 }
 
 /// A meta-level map containing sections.
 class MultiSectionMapping extends Mapping {
@@ skipping 69 matching lines @@
           ..write(':')
           ..write(_maps[i])
           ..write(')');
     }
     buff.write(']');
     return buff.toString();
   }
 }
 
 /// A map containing direct source mappings.
 // TODO(sigmund): integrate mapping and sourcemap builder?

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

delete TODO :-)

[tjblasi, 2014/07/08 16:36:19]

Done.

 class SingleMapping extends Mapping {
   /// Url of the target file.
   final String targetUrl;
 
   /// Source urls used in the mapping, indexed by id.
   final List<String> urls;
 
   /// Source names used in the mapping, indexed by id.
   final List<String> names;
 
   /// Entries indicating the beginning of each span.
   final List<TargetLineEntry> lines = <TargetLineEntry>[];
 
+  SingleMapping._internal(this.targetUrl, this.urls, this.names);
+
+  factory SingleMapping.fromBuilder(SourceMapBuilder builder, [String fileUrl]) {

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

how about switching to take a list of entries instead of a builder?

[tjblasi, 2014/07/08 16:36:20]

Done.

+    List<Entry> sourceEntries = builder.entries;
+    List<TargetLineEntry> lines = [];

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

style nit: use 'var' for local variables

[tjblasi, 2014/07/08 16:36:20]

Done.

+
+    // Indices associated with file urls that will be part of the source map. We
+    // use a linked hash-map so that `_urls.keys[_urls[u]] == u`
+    Map<String, int> urls = new LinkedHashMap<String, int>();

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

same here and below (var urls = ...)

[tjblasi, 2014/07/08 16:36:20]

Done.

+
+    // Indices associated with identifiers that will be part of the source map.
+    // We use a linked hash-map so that `_names.keys[_names[n]] == n`
+    Map<String, int> names = new LinkedHashMap<String, int>();
+
+    // The encoding needs to be sorted by the target offsets.
+    sourceEntries.sort();

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

since sorting modifies the list in-place, we might want to make a copy of the
list at the beginning. I think something like:
  entries = entries.toList();

will do.

[tjblasi, 2014/07/08 16:36:20]

Done.

+    for (var sourceEntry in sourceEntries) {
+      if (lines.isEmpty || sourceEntry.target.line > lines.last.line) {
+        lines.add(new TargetLineEntry(sourceEntry.target.line, []));
+      }
+
+      if (sourceEntry.source == null) {
+        lines.last.entries.add(new TargetEntry(sourceEntry.target.column));

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

since `lines.last` can be expensive, maybe we could follow a similar pattern as
the `fromJson` constructor and have a couple variables around for these commonly
accessed properties.

For example, I'd consider having `line` to track the current line and a list of
`entries` of the current line, both of which can be reset above when you
determine that the target.line changed.

[tjblasi, 2014/07/08 16:36:20]

Done.

+      } else {
+        var urlId = urls.putIfAbsent(sourceEntry.source.sourceUrl, () => urls.length);

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

nit: 80 columns (here and elsewhere)

[tjblasi, 2014/07/08 16:36:20]

Done.

+        var srcNameId = sourceEntry.identifierName != null ?

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

minor nit: maybe swapping this branch might make it a bit easier to read this
expression.

var srcNameId = sourceEntry.identifierName == null ? null :
    names.putIfAbsent(sourceEntry.identifierName, () => names.length);

Unlike other operators, when it helps I wrap before the ? : operators. Not sure
if matters here though:

var srcNameId = sourceEntry.identifierName == null
    ? null
    : names.putIfAbsent(sourceEntry.identifierName, () => names.length);

[tjblasi, 2014/07/08 16:36:20]

Done.

+            names.putIfAbsent(sourceEntry.identifierName, () => names.length) : null;
+        lines.last.entries.add(new TargetEntry(
+            sourceEntry.target.column,
+            urlId,
+            sourceEntry.source.line,
+            sourceEntry.source.column,
+            srcNameId));
+      }
+    }
+    var mapping = new SingleMapping._internal(fileUrl, urls.keys.toList(), names.keys.toList());
+    mapping.lines.addAll(lines);

[Siggi Cherem (dart-lang), 2014/07/08 00:31:01]

nit: maybe move the default initialization above to the fromJson constructor,
and add 'lines' also as an argument for the _internal constructor?

[tjblasi, 2014/07/08 16:36:20]

Done.

+    return mapping;
+  }
+
   SingleMapping.fromJson(Map map)
       : targetUrl = map['file'],
         // TODO(sigmund): add support for 'sourceRoot'
         urls = map['sources'],
         names = map['names'] {
     int line = 0;
     int column = 0;
     int srcUrlId = 0;
     int srcLine = 0;
     int srcColumn = 0;
@@ skipping 50 matching lines @@
               srcNameId));
         }
       }
       if (tokenizer.nextKind.isNewSegment) tokenizer._consumeNewSegment();
     }
     if (!entries.isEmpty) {
       lines.add(new TargetLineEntry(line, entries));
     }
   }
 
+  /// Encodes the Mapping mappings as a json map.
+  Map toMap() {

[Siggi Cherem (dart-lang), 2014/07/08 00:31:00]

suggestion: maybe rename as toJson (to make it contrast the 'fromJson'
constructor)

[tjblasi, 2014/07/08 16:36:20]

Done.

+    var buff = new StringBuffer();
+    var line = 0;
+    var column = 0;
+    var srcLine = 0;
+    var srcColumn = 0;
+    var srcUrlId = 0;
+    var srcNameId = 0;
+    var first = true;
+
+    for (var entry in lines) {
+      int nextLine = entry.line;
+      if (nextLine > line) {
+        for (int i = line; i < nextLine; ++i) {
+          buff.write(';');
+        }
+        line = nextLine;
+        column = 0;
+        first = true;
+      }
+
+      for (var segment in entry.entries) {
+        if (!first) buff.write(',');
+        first = false;
+        column = _append(buff, column, segment.column);
+
+        // Encoding can be just the column offset if there is no source
+        // information.
+        var newUrlId = segment.sourceUrlId;
+        if (newUrlId == null) continue;
+        srcUrlId = _append(buff, srcUrlId, newUrlId);
+        srcLine = _append(buff, srcLine, segment.sourceLine);
+        srcColumn = _append(buff, srcColumn, segment.sourceColumn);
+
+        if (segment.sourceNameId == null) continue;
+        srcNameId = _append(buff, srcNameId, segment.sourceNameId);
+      }
+    }
+
+    var result = {
+      'version': 3,
+      'sourceRoot': '',
+      'sources': urls,
+      'names' : names,
+      'mappings' : buff.toString()
+    };
+    if (targetUrl != null) {
+      result['file'] = targetUrl;
+    }
+    return result;
+  }
+
+  /// Appends to [buff] a VLQ encoding of [newValue] using the difference
+  /// between [oldValue] and [newValue]
+  static int _append(StringBuffer buff, int oldValue, int newValue) {
+    buff.writeAll(encodeVlq(newValue - oldValue));
+    return newValue;
+  }
+
   _segmentError(int seen, int line) => new StateError(
       'Invalid entry in sourcemap, expected 1, 4, or 5'
       ' values, but got $seen.\ntargeturl: $targetUrl, line: $line');
 
   /// Returns [TargetLineEntry] which includes the location in the target [line]
   /// number. In particular, the resulting entry is the last entry whose line
   /// number is lower or equal to [line].
   TargetLineEntry _findLine(int line) {
     int index = binarySearch(lines, (e) => e.line > line);
     return (index <= 0) ? null : lines[index - 1];
@@ skipping 153 matching lines @@
   static const _TokenKind EOF = const _TokenKind(isEof: true);
   static const _TokenKind VALUE = const _TokenKind();
   final bool isNewLine;
   final bool isNewSegment;
   final bool isEof;
   bool get isValue => !isNewLine && !isNewSegment && !isEof;
 
   const _TokenKind(
       {this.isNewLine: false, this.isNewSegment: false, this.isEof: false});
 }