Support external implementations of FileSpan.

Support external implementations of FileSpan.

Previously, several FileSpan methods assumed that all FileSpans were
implementations of this package's FileSpan class. However, this may not
always be true. It can be useful to create FileSpans without first
creating a SourceFile when efficient representations of line/column
information are already available, and the user doesn't want to eagerly
do the parsing necessary to create a full SourceFile.

This also fixes an inconsistency between FileSpan.== and
FileSpan.hashCode.

R=sigmund@google.com

Committed: https://github.com/dart-lang/source_span/commit/80c947f98cc290b4d95a46c829d3efbbb9560746

[nweiz, 2015-08-18 02:04:24]

nweiz@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-08-18 18:01:59]

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart
File lib/src/file.dart (right):

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart#newcode221
lib/src/file.dart:221: if (other.runtimeType != FileSpan) return
super.compareTo(other);
Could we instead move some of the shared code into a mixin where it is safe to
do an `is` check?

We rarely use this code in client apps, but `.runtimeType` makes code more
expensive for dart2js.

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart#newcode249
lib/src/file.dart:249: return _start == other._start && _end == other._end;
mmm - I thought we would keep the check on sourceUrl here. Should we add it
back? Should we also add sourceUrl to SourceSpanMixin.== and
SourceSpanMixin.hashCode (fixing the inconsistency by adding that there
instead)?

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart#newcode264
lib/src/file.dart:264: var start = math.min(this._start, other._start);
I wonder if we should have these optimizations or switch to just use
other.start.offset here directly (assuming we change the start & end getters to
memoize the FileLocations results, so we allocate them just once).

[nweiz, 2015-08-18 20:28:51]

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart
File lib/src/file.dart (right):

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart#newcode221
lib/src/file.dart:221: if (other.runtimeType != FileSpan) return
super.compareTo(other);
On 2015/08/18 18:01:59, Siggi Cherem (dart-lang) wrote:
> Could we instead move some of the shared code into a mixin where it is safe to
> do an `is` check?
> 
> We rarely use this code in client apps, but `.runtimeType` makes code more
> expensive for dart2js.

I don't think a mixin would work, but I've split out an implementation class.

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart#newcode249
lib/src/file.dart:249: return _start == other._start && _end == other._end;
On 2015/08/18 18:01:59, Siggi Cherem (dart-lang) wrote:
> mmm - I thought we would keep the check on sourceUrl here. Should we add it
> back?

Yeah, good catch. Just a cut/paste error.

> Should we also add sourceUrl to SourceSpanMixin.== and
> SourceSpanMixin.hashCode (fixing the inconsistency by adding that there
> instead)?

I'm not sure why I didn't have sourceUrl always factor into equality, but
changing it now would break backwards compatibility :-/.

https://codereview.chromium.org/1298093002/diff/1/lib/src/file.dart#newcode264
lib/src/file.dart:264: var start = math.min(this._start, other._start);
On 2015/08/18 18:01:59, Siggi Cherem (dart-lang) wrote:
> I wonder if we should have these optimizations or switch to just use
> other.start.offset here directly (assuming we change the start & end getters
to
> memoize the FileLocations results, so we allocate them just once).

In my experiments, there's a lot of benefit that comes from avoiding the
creation of FileLocations until the last minute. It's pretty common to create
and even expand spans during the course of parsing, and adding two extra
allocations for each expand is a real cost. I want to keep the non-error-case
cost of using this library as low as possible to encourage parsers to expose the
information.

[nweiz, 2015-08-18 20:29:02]

Code review changes

[nweiz, 2015-08-18 20:29:58]

Code review changes

[Siggi Cherem (dart-lang), 2015-08-18 21:38:36]

lgtm

https://codereview.chromium.org/1298093002/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1298093002/diff/40001/CHANGELOG.md#newcode6
CHANGELOG.md:6: * `FileSpan.hashCode` now fully agrees with `FileSpan.==`.
I think before that was true too, maybe it's more that FileSpan.hashCode agrees
with SourceSpan.hashCode?

https://codereview.chromium.org/1298093002/diff/40001/lib/src/file.dart
File lib/src/file.dart (right):

https://codereview.chromium.org/1298093002/diff/40001/lib/src/file.dart#newco...
lib/src/file.dart:235: // Check runtimeType to be resilient to external FileSpan
implementations.
remove comment now that it's no longer needed (same in the two places below)

[nweiz, 2015-08-18 22:01:17]

Code review changes

[nweiz, 2015-08-18 22:01:25]

https://codereview.chromium.org/1298093002/diff/40001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1298093002/diff/40001/CHANGELOG.md#newcode6
CHANGELOG.md:6: * `FileSpan.hashCode` now fully agrees with `FileSpan.==`.
On 2015/08/18 21:38:36, Siggi Cherem (dart-lang) wrote:
> I think before that was true too, maybe it's more that FileSpan.hashCode
agrees
> with SourceSpan.hashCode?

Before, a FileSpan == a SourceSpan with the same start/end, but the hash codes
weren't equal.

https://codereview.chromium.org/1298093002/diff/40001/lib/src/file.dart
File lib/src/file.dart (right):

https://codereview.chromium.org/1298093002/diff/40001/lib/src/file.dart#newco...
lib/src/file.dart:235: // Check runtimeType to be resilient to external FileSpan
implementations.
On 2015/08/18 21:38:36, Siggi Cherem (dart-lang) wrote:
> remove comment now that it's no longer needed (same in the two places below)

Done.

[nweiz, 2015-08-18 22:01:38]

Committed patchset #4 (id:60001) manually as
80c947f98cc290b4d95a46c829d3efbbb9560746 (presubmit successful).