Support unmapped areas in source maps.

Support unmapped areas in source maps.

R=johnniwinther@google.com

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

[zarah, 2014-04-14 16:45:24]

While working with validating the source maps generated by dart2js I encountered
the problem of handling unmapped areas. Before this change, the parser would
always add 4-field segments, thereby not being able to represent gaps in a line.
 

In this CL I try to fix this problem by changing the parser and the source map
builder. Siggi: are you okay with this change or does it conflict with your
understanding?

[Johnni Winther, 2014-04-15 06:50:19]

LGTM but wait for Siggi's approval.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
File pkg/source_maps/test/parse_map_with_no_source_location_test.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:12: const
Map<String, dynamic>MAP_WITH_NO_SOURCE_LOCATION = const {
Add space between type and variable.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:21: const
Map<String, dynamic>MAP_WITH_SOURCE_LOCATION = const {
Ditto.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:30: const
Map<String, dynamic>MAP_WITH_SOURCE_LOCATION_AND_NAME = const {
Ditto.

[Siggi Cherem (dart-lang), 2014-04-18 02:00:31]

Hi Sarah,

Thanks for sending this change! I had this in my backlog for a very long time
(see https://code.google.com/p/dart/issues/detail?id=15124)

Overall the changes look great. In fact they are pretty similar to the change I
had prepared locally. I just came back from vacation, but I'll try to spend time
this week looking into what I was mentioned in that bug.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/buil...
File pkg/source_maps/lib/builder.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/buil...
pkg/source_maps/lib/builder.dart:120: if (oldValue == null) {
I might be missing something, could we simply revert this line and all the
initializations in line 54 back to 0 instead of null?

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
File pkg/source_maps/lib/parser.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
pkg/source_maps/lib/parser.dart:201: sourceUrlId: srcUrlId,
nit: just to match the style we use in this package, let's wrap this with just
+4 and put as many as we can on the same line. Alternatively, we can keep this
as before and used positional optional arguments instead. I'd be happy either
way

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
pkg/source_maps/lib/parser.dart:211: new TargetEntry(column,
similarly here. We can also move this 'new ..." to the prev line since we'll
anyways wrap around the arguments.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
File pkg/source_maps/test/parse_map_with_no_source_location_test.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:5: library
test.parser_test;
let's move these tests into parser_test.dart, as 3 more 'test()'s

[Siggi Cherem (dart-lang), 2014-04-22 18:12:44]

I looked more and can't find anything that will let me reproduce the original
issue that led me to introduce the bug in the first place. So I think we should
go ahead with your fixes.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
File pkg/source_maps/lib/parser.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
pkg/source_maps/lib/parser.dart:201: sourceUrlId: srcUrlId,
On 2014/04/18 02:00:31, Siggi Cherem (dart-lang) wrote:
> nit: just to match the style we use in this package, let's wrap this with just
> +4 and put as many as we can on the same line. Alternatively, we can keep this
> as before and used positional optional arguments instead. I'd be happy either
> way

Thinking more about it, let's switch back to positional optional arguments, that
way we can release this as a minor release instead of a breaking API release.
With a minor release, most people will get the bug fix without having to update
their version dependencies.

We need to update the version in pubspec.yaml. We have two options: use version
"0.9.1-dev", or use "0.9.1" and release soon after submitting the change.

[zarah, 2014-04-23 07:52:12]

Thanks for the comments!

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/buil...
File pkg/source_maps/lib/builder.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/buil...
pkg/source_maps/lib/builder.dart:120: if (oldValue == null) {
On 2014/04/18 02:00:31, Siggi Cherem (dart-lang) wrote:
> I might be missing something, could we simply revert this line and all the
> initializations in line 54 back to 0 instead of null?

I don't think you are :-), Done.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
File pkg/source_maps/lib/parser.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
pkg/source_maps/lib/parser.dart:201: sourceUrlId: srcUrlId,
On 2014/04/18 02:00:31, Siggi Cherem (dart-lang) wrote:
> nit: just to match the style we use in this package, let's wrap this with just
> +4 and put as many as we can on the same line. Alternatively, we can keep this
> as before and used positional optional arguments instead. I'd be happy either
> way

Done.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
pkg/source_maps/lib/parser.dart:201: sourceUrlId: srcUrlId,
On 2014/04/22 18:12:44, Siggi Cherem (dart-lang) wrote:
> On 2014/04/18 02:00:31, Siggi Cherem (dart-lang) wrote:
> > nit: just to match the style we use in this package, let's wrap this with
just
> > +4 and put as many as we can on the same line. Alternatively, we can keep
this
> > as before and used positional optional arguments instead. I'd be happy
either
> > way
> 
> Thinking more about it, let's switch back to positional optional arguments,
that
> way we can release this as a minor release instead of a breaking API release.
> With a minor release, most people will get the bug fix without having to
update
> their version dependencies.
> 
> We need to update the version in pubspec.yaml. We have two options: use
version
> "0.9.1-dev", or use "0.9.1" and release soon after submitting the change.

Updated to 0.9.1-dev .

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/lib/pars...
pkg/source_maps/lib/parser.dart:211: new TargetEntry(column,
On 2014/04/18 02:00:31, Siggi Cherem (dart-lang) wrote:
> similarly here. We can also move this 'new ..." to the prev line since we'll
> anyways wrap around the arguments.

Done.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
File pkg/source_maps/test/parse_map_with_no_source_location_test.dart (right):

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:5: library
test.parser_test;
On 2014/04/18 02:00:31, Siggi Cherem (dart-lang) wrote:
> let's move these tests into parser_test.dart, as 3 more 'test()'s 

Done.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:12: const
Map<String, dynamic>MAP_WITH_NO_SOURCE_LOCATION = const {
On 2014/04/15 06:50:19, Johnni Winther wrote:
> Add space between type and variable.

Done.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:21: const
Map<String, dynamic>MAP_WITH_SOURCE_LOCATION = const {
On 2014/04/15 06:50:19, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/237123003/diff/20001/pkg/source_maps/test/par...
pkg/source_maps/test/parse_map_with_no_source_location_test.dart:30: const
Map<String, dynamic>MAP_WITH_SOURCE_LOCATION_AND_NAME = const {
On 2014/04/15 06:50:19, Johnni Winther wrote:
> Ditto.

Done.

[zarah, 2014-04-23 07:57:25]

Committed patchset #2 manually as r35307 (presubmit successful).