Make SourceInfo a class, rename its menthods.

Make SourceInfo a class, rename its menthods.
This is preparation for avoiding calling Element.getNode() during analysis.


R=brianwilkerson@google.com,zundel@google.com
BUG=
TEST=

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

[Brian Wilkerson, 2012-03-08 22:52:04]

https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/common/SourceInfo.java (right):

https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/common/SourceInfo.java:38: private static
final Map<Source, LinesInfo> lines = new MapMaker().weakKeys().makeMap();
Why is it better to have a static field in this class that holds this
information rather than just allowing each Source to hold a (lazily initialized)
reference to its LinesInfo? If the lifetime of the LinesInfo is limited to the
lifetime of the Source, then it would make more sense (less error prone).

https://chromiumcodereview.appspot.com/9651004/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/dom/rewrite/LineCommentEndOffsets.java
(right):

https://chromiumcodereview.appspot.com/9651004/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/dom/rewrite/LineCommentEndOffsets.java:37:
if (offset < 0 || offset < content.length
nit: In most other places I haven't cared so much about the removal of
parentheses (and liked it better without the extras in some cases), but I think
it's much more clear here to have the extra parentheses, especially because of
the unfortunate formatting. Please add them back here.

[scheglov, 2012-03-08 23:28:31]

https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/common/SourceInfo.java (right):

https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/common/SourceInfo.java:38: private static
final Map<Source, LinesInfo> lines = new MapMaker().weakKeys().makeMap();
On 2012/03/08 22:52:04, Brian Wilkerson wrote:
> Why is it better to have a static field in this class that holds this
> information rather than just allowing each Source to hold a (lazily
initialized)
> reference to its LinesInfo? If the lifetime of the LinesInfo is limited to the
> lifetime of the Source, then it would make more sense (less error prone).

Yes, the lifetime of LinesInfo is same as lifetime of the Source.
Knowing only line/column without knowing source file is useless.

Static field allows us:
1. share LinesInfo between SourceInfo instances for the same Source;
2. don't have separate field in the class instance (which also consumes space,
and we have a lot of SourceInfo instances).

But you are right, in the most cases we don't need line/column at all, so we can
prepare LinesInfo lazily.

https://chromiumcodereview.appspot.com/9651004/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/dom/rewrite/LineCommentEndOffsets.java
(right):

https://chromiumcodereview.appspot.com/9651004/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/dom/rewrite/LineCommentEndOffsets.java:37:
if (offset < 0 || offset < content.length
On 2012/03/08 22:52:04, Brian Wilkerson wrote:
> nit: In most other places I haven't cared so much about the removal of
> parentheses (and liked it better without the extras in some cases), but I
think
> it's much more clear here to have the extra parentheses, especially because of
> the unfortunate formatting. Please add them back here.

Done.

[Brian Wilkerson, 2012-03-08 23:49:23]

https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/common/SourceInfo.java (right):

https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/common/SourceInfo.java:38: private static
final Map<Source, LinesInfo> lines = new MapMaker().weakKeys().makeMap();
> Static field allows us:
> 1. share LinesInfo between SourceInfo instances for the same Source;

Associating the LinesInfo with the Source would have the same benefit.

> 2. don't have separate field in the class instance (which also consumes space,
> and we have a lot of SourceInfo instances).

Given what I was suggesting, there would only be one extra field per Source, not
one per SourceInfo, and it would consume less space than a hash map entry for
those sources that have LinesInfo computed for them. Depending on the number of
source files it might even be less space than an empty HashMap.

[scheglov, 2012-03-09 00:47:16]

On 2012/03/08 23:49:23, Brian Wilkerson wrote:
>
https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
> File compiler/java/com/google/dart/compiler/common/SourceInfo.java (right):
> 
>
https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
> compiler/java/com/google/dart/compiler/common/SourceInfo.java:38: private
static
> final Map<Source, LinesInfo> lines = new MapMaker().weakKeys().makeMap();
> > Static field allows us:
> > 1. share LinesInfo between SourceInfo instances for the same Source;
> 
> Associating the LinesInfo with the Source would have the same benefit.

  Do you want to put getLine(offset)/getColumn(offset) methods into Source
class?

[scheglov, 2012-03-09 00:48:38]

On 2012/03/08 23:49:23, Brian Wilkerson wrote:
>
https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
> File compiler/java/com/google/dart/compiler/common/SourceInfo.java (right):
> 
>
https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
> compiler/java/com/google/dart/compiler/common/SourceInfo.java:38: private
static
> final Map<Source, LinesInfo> lines = new MapMaker().weakKeys().makeMap();
> > Static field allows us:
> > 1. share LinesInfo between SourceInfo instances for the same Source;
> 
> Associating the LinesInfo with the Source would have the same benefit.

  Do you want to put getLine(offset)/getColumn(offset) methods into Source
class?

[scheglov, 2012-03-09 00:56:00]

On 2012/03/09 00:48:38, scheglov wrote:
> On 2012/03/08 23:49:23, Brian Wilkerson wrote:
> >
>
https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
> > File compiler/java/com/google/dart/compiler/common/SourceInfo.java (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9651004/diff/1/compiler/java/com/googl...
> > compiler/java/com/google/dart/compiler/common/SourceInfo.java:38: private
> static
> > final Map<Source, LinesInfo> lines = new MapMaker().weakKeys().makeMap();
> > > Static field allows us:
> > > 1. share LinesInfo between SourceInfo instances for the same Source;
> > 
> > Associating the LinesInfo with the Source would have the same benefit.
> 
>   Do you want to put getLine(offset)/getColumn(offset) methods into Source
> class?

And actually I don't like this.
There was no need AFAIK for this functionality before, line/column was
accessible only from DartNode.
So, I'd prefer to keep it in the place where we actually hold all location
information, i.e. in SourceInfo.

Plus, there are many implementations of the Source, would be not nice to
implement getLine()/getColumn() in every of them.

[Brian Wilkerson, 2012-03-09 01:12:50]

On 2012/03/09 00:56:00, scheglov wrote:
> On 2012/03/09 00:48:38, scheglov wrote:
> Do you want to put getLine(offset)/getColumn(offset) methods
> into Source class?

No, just getLinesInfo().

> And actually I don't like this.
> There was no need AFAIK for this functionality before, line/column was
> accessible only from DartNode.
> So, I'd prefer to keep it in the place where we actually hold all location
> information, i.e. in SourceInfo.

Understood, but I really don't like having tightly coupled information being
stored in a loose association like this.

> Plus, there are many implementations of the Source, would be not nice to
> implement getLine()/getColumn() in every of them.

Seems like we could add an abstract superclass that could implement this
functionality.

Oh well, I guess we'll just have to agree to disagree for now. It isn't worth
holding up the check-in while we discuss it, so LGTM.