Remove unnecessary List creation

Remove unnecessary List creation

R=scheglov@google.com

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

[Brian Wilkerson, 2014-10-26 04:42:19]

brianwilkerson@google.com changed reviewers:
+ scheglov@google.com

[Brian Wilkerson, 2014-10-26 04:42:20]

There are more places where I think I can get rid of unnecessary List creations,
but they'll be more involved and I thought this was already a big enough CL.

[scheglov, 2014-10-26 04:52:42]

LGTM

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:5105: => new
List.from(_dataMap.values);
Or just _dataMap.values.toList()

[sra1, 2014-10-26 06:31:38]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2014-10-26 06:31:39]

DBC

https://codereview.chromium.org/677303002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/generated/completion.dart (right):

https://codereview.chromium.org/677303002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/generated/completion.dart:821:
List<Element> elements = <Element>[];
This local is shadowing the parameter.
The original code was using a new list instead of passing in a list.

Both are suspicious - the parameter has no purpose, and the function always
returns a new empty growable list.
Is there a unit test that would catch this?

The code should probably be using 
  elements.removeWhere((e) => ...);
  return elements;
to mutate the input, or
  return elements.where((e) => ...).toList();
to return a new list with some elements filtered out.

If the code should be mutating, I would make the return type void and use a
cascade at the call site, like the 'sort' example in another comment.

https://codereview.chromium.org/677303002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/generated/completion.dart:844:
List<Element> elements = <Element>[];
ditto

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/ast.dart:204: return children;
or 

return <AstNode>[]
  ..add(_comment)
  ..addAll(_metadata)
  ..sort(AstNode.LEXICAL_ORDER);

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:5105: => new
List.from(_dataMap.values);
On 2014/10/26 04:52:42, scheglov wrote:
> Or just _dataMap.values.toList()

It is often better to use .toList() since it carries the generic type parameter
without having to write it.

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/resolver.dart (left):

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/resolver.dart:424: List<AngularPropertyElement>
_parseComponentProperties() {
It would be nice if this file was split into smaller files to help our feeble
code review tools.

[Brian Wilkerson, 2014-10-26 15:15:46]

https://codereview.chromium.org/677303002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/generated/completion.dart (right):

https://codereview.chromium.org/677303002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/generated/completion.dart:821:
List<Element> elements = <Element>[];
Good catch! The original Java code was mistranslated. The original makes a copy,
so I used 'where'.

I'll work with Dan and Konstantin to figure out how best to test this.

https://codereview.chromium.org/677303002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/generated/completion.dart:844:
List<Element> elements = <Element>[];
Same problem, same solution.

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/ast.dart:204: return children;
Done. I am more concerned at the moment about performance issues than I am about
making the code more Dart-like, but since you mentioned it...

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:5105: => new
List.from(_dataMap.values);
> It is often better to use .toList() since it carries the generic type
parameter
> without having to write it.

Thanks! I was wondering whether there was a reason to choose one over the other.
I'll change this occurrence before committing, then I'll convert the rest in a
later CL.

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/resolver.dart (left):

https://codereview.chromium.org/677303002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/resolver.dart:424: List<AngularPropertyElement>
_parseComponentProperties() {
> It would be nice if this file was split into smaller files to help our feeble
> code review tools.

Agreed. That's another task on my list, and another CL.

[Brian Wilkerson, 2014-10-26 15:18:00]

PTAL

[scheglov, 2014-10-26 16:36:06]

LGTM

[Brian Wilkerson, 2014-10-27 17:41:55]

Committed patchset #2 (id:20001) manually as 41327 (presubmit successful).