Improvements to task model

Improvements to task model

R=paulberry@google.com, scheglov@google.com

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

[Brian Wilkerson, 2015-01-26 04:55:23]

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

[scheglov, 2015-01-26 16:10:34]

LGTM

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/ge...
File pkg/analyzer/lib/src/task/general.dart (right):

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/ge...
pkg/analyzer/lib/src/task/general.dart:22: GetContentTask.createTask,
FYI, we don't need GetContentTask qualifier now.
Just in case if it wasn't an intentional decision.

[Brian Wilkerson, 2015-01-26 16:14:13]

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/ge...
File pkg/analyzer/lib/src/task/general.dart (right):

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/ge...
pkg/analyzer/lib/src/task/general.dart:22: GetContentTask.createTask,
> Just in case if it wasn't an intentional decision.

It wasn't. Thanks! I've removed them.

[Paul Berry, 2015-01-26 17:31:06]

Comments below based on the in person discussion we just had.  I'm happy to
leave it to your judgement whether to make any changes or not, so lgtm.

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/mo...
File pkg/analyzer/lib/src/task/model.dart (right):

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/mo...
pkg/analyzer/lib/src/task/model.dart:25: ContributionPointImpl(String name, V
defaultValue)
Since the "value" associated with a contribution point is always determined by
concatenating together the results of all the tasks that contribute to it, does
it even make sense for a contribution point to have a default value?  Maybe we
should remove the "defaultValue" argument and just pass "null" to super.

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/mo...
pkg/analyzer/lib/src/task/model.dart:48: final V defaultValue;
This design forces all results to share the same default value.  If the values
stored in results were immutable data structures, I would be ok with this, but
since we sometimes mutate them (e.g. when annotating an AST with pointers to the
element model), it feels a little dangerous.

Would it be better to instead store a function having type () -> V, so that we
can use it to create a default value on the fly when we need it?

[Brian Wilkerson, 2015-01-26 19:47:24]

Committed patchset #1 (id:1) manually as 43146 (presubmit successful).

[Brian Wilkerson, 2015-01-26 19:47:53]

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/mo...
File pkg/analyzer/lib/src/task/model.dart (right):

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/mo...
pkg/analyzer/lib/src/task/model.dart:25: ContributionPointImpl(String name, V
defaultValue)
> ... does it even make sense for a contribution point to have a default value?

I think it doesn't. I'll make the change.

https://codereview.chromium.org/867563004/diff/1/pkg/analyzer/lib/src/task/mo...
pkg/analyzer/lib/src/task/model.dart:48: final V defaultValue;
> This design forces all results to share the same default value.  ...  it feels
a little dangerous.

Good point! It isn't a problem at the moment because all of the results whose
values can be mutable data structures currently have a default value of 'null',
but we should definitely revisit this, given that plugins will be contributing
result descriptors.

[Paul Berry, 2015-01-27 00:08:45]

I'm now seeing errors and warnings in analyzer/test/src/task/test_all.dart:

Target of URI does not exist: 'dart_test.dart'
Undefined name 'dart_test'
Undefined name 'dart_test'

Did you forget to include the file "dart_test.dart"?

[Brian Wilkerson, 2015-01-27 00:51:13]

No, I forgot to back out that change that accidentally got into the file. I'll
fix it when I get home if someone doesn't do it first.