Resource/File/Folder interfaces and implementations for dart:io and memory.

Resource/File/Folder interfaces and implementations for dart:io and memory.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2014-05-25 15:29:57]

LGTM

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/resource.dart (right):

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:89: bool operator ==(o) {
I really dislike single character variable names for anything other than loop
variables because I believe that names should carry semantic meaning. Even the
overridden method uses "other".

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:195: List<Resource> children = [];
I think we should include a type annotation with literal lists and maps because
it has meaning in checked mode (that is "[]" --> "<Resource>[]"). (Also in other
places.)

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:232: throw new ArgumentError('Path
must start with \'/\'');
It would be better to use double quotes so that you don't need to escape the
embedded single quotes.

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:235: String partialPath = "";
It's probably more efficient to use a StringBuffer to build up the string.

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:251: message += 'but
${resource.runtimeType} found';
It looks really strange to have a mixture of "+" and string interpolation to
build the same string. Is there a reason for this?

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/test/res...
File pkg/analysis_server/test/resource_test.dart (right):

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/test/res...
pkg/analysis_server/test/resource_test.dart:128: group('_MemoryFileSource', () {
It would be good to also test a non-existent file.

[scheglov, 2014-05-25 16:16:38]

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/resource.dart (right):

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:89: bool operator ==(o) {
On 2014/05/25 15:29:58, Brian Wilkerson wrote:
> I really dislike single character variable names for anything other than loop
> variables because I believe that names should carry semantic meaning. Even the
> overridden method uses "other".

Done.

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:195: List<Resource> children = [];
On 2014/05/25 15:29:58, Brian Wilkerson wrote:
> I think we should include a type annotation with literal lists and maps
because
> it has meaning in checked mode (that is "[]" --> "<Resource>[]"). (Also in
other
> places.)

Done.

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:232: throw new ArgumentError('Path
must start with \'/\'');
On 2014/05/25 15:29:58, Brian Wilkerson wrote:
> It would be better to use double quotes so that you don't need to escape the
> embedded single quotes.

Done.

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:235: String partialPath = "";
On 2014/05/25 15:29:58, Brian Wilkerson wrote:
> It's probably more efficient to use a StringBuffer to build up the string.

It is surprising, but no, it is not faster.
Actually with StringBuffer.write() works 10 times (!) slower than String+.

main() {
  var sw = new Stopwatch();
  var totalLength = 0;
  for (int j = 0; j < 1000000; j++) {
    sw.start();
    {
      String s = 'aaa';
      s += 'bbb';
      s += 'ccc';
      s += 'ddd';
      s += 'eee';
      totalLength += s.length;
    }
    // StringBuffer.write() is 10x slower
//    {
//      var sb = new StringBuffer();
//      // writeAll() is even slower
//      sb.writeAll(['aaa', 'bbb', 'ccc', 'ddd', 'eee']);
//      sb.write('aaa');
//      sb.write('bbb');
//      sb.write('ccc');
//      sb.write('ddd');
//      sb.write('eee');
//      totalLength += sb.toString().length;
//    }
    sw.stop();
  }
  print(sw.elapsedMilliseconds);
  print(totalLength);
}

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:251: message += 'but
${resource.runtimeType} found';
On 2014/05/25 15:29:58, Brian Wilkerson wrote:
> It looks really strange to have a mixture of "+" and string interpolation to
> build the same string. Is there a reason for this?

Mostly because it does not fit one 80 characters line.
But yes, it can be made better by using AdjacentStrings.

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/test/res...
File pkg/analysis_server/test/resource_test.dart (right):

https://codereview.chromium.org/299403004/diff/1/pkg/analysis_server/test/res...
pkg/analysis_server/test/resource_test.dart:128: group('_MemoryFileSource', () {
On 2014/05/25 15:29:58, Brian Wilkerson wrote:
> It would be good to also test a non-existent file.

Done.

[scheglov, 2014-05-25 16:17:49]

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

[Paul Berry, 2014-05-27 15:56:11]

lgtm