Library children object fix

Library children object fix

R=alanknight@google.com, jacobr@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/ef03e00f4441fa8eb02fea0115b89d4be7e9f08e

[bmilligan, 2016-07-25 17:26:08]

bmilligan@google.com changed reviewers:
+ alanknight@google.com, jacobr@google.com

[Alan Knight, 2016-07-25 19:51:38]

lgtm

https://codereview.chromium.org/2176233002/diff/20001/tool/input_sdk/private/...
File tool/input_sdk/private/debugger.dart (right):

https://codereview.chromium.org/2176233002/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:580: children.add(new NameValuePair(name:
name, value: value));
This code is complicated enough it might be worth looking at how to split it up
to make it clearer.

[bmilligan, 2016-07-25 20:23:54]

https://codereview.chromium.org/2176233002/diff/20001/tool/input_sdk/private/...
File tool/input_sdk/private/debugger.dart (right):

https://codereview.chromium.org/2176233002/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:580: children.add(new NameValuePair(name:
name, value: value));
On 2016/07/25 19:51:38, Alan Knight wrote:
> This code is complicated enough it might be worth looking at how to split it
up
> to make it clearer.

Done.

[Alan Knight, 2016-07-25 20:53:00]

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/debugger.dart (right):

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:557: if (value != null) {
You could even split this up more/ reduce the level of nesting if you used
iterables. e.g.

safeProperties(entry) => getOwnPropertyNames(entry).map((name) =>
safeGetProperty(entry, name)).where((each) => each != null);

for (var value in safeProperties(entry)) {
 ....
}

There are some interesting questions, though. That works well for creating the
children, but you have the generic constructor issue, which has a side effect
and doesn't add to the list. And what happens if there's more than one generic
constructor in a class?

I might be tempted to split this into two loops, one for type parameters and one
for children. That's less efficient, but the number of children for any
non-collection type should be quite small, and I assume that for collections
we're already doing something to limit it.

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:562: addClassChild(name, value, children);
Good, but maybe for consistency with the clause below, do it more like

   children.add(classChild(name, value))

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:571: genericClassHandler(String name,
Object genericTypeConstructor) {
"Handler" is another one of those non-value-adding words. Also, better to name
methods as verbs than nouns. Better to call this something like noteGenerics or
recordTypeParameters

[bmilligan, 2016-07-25 23:28:31]

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/debugger.dart (right):

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:557: if (value != null) {
On 2016/07/25 20:53:00, Alan Knight wrote:
> You could even split this up more/ reduce the level of nesting if you used
> iterables. e.g.
> 
> safeProperties(entry) => getOwnPropertyNames(entry).map((name) =>
> safeGetProperty(entry, name)).where((each) => each != null);
> 
> for (var value in safeProperties(entry)) {
>  ....
> }
> 
> There are some interesting questions, though. That works well for creating the
> children, but you have the generic constructor issue, which has a side effect
> and doesn't add to the list. And what happens if there's more than one generic
> constructor in a class?
> 
> I might be tempted to split this into two loops, one for type parameters and
one
> for children. That's less efficient, but the number of children for any
> non-collection type should be quite small, and I assume that for collections
> we're already doing something to limit it.

The generic types that get caught in the the first part of the if statement do
not want to be added as children, only their parameters saved. So, I could
iterate through twice, but I would have to make the same check in both for that
generic type to make sure that in the "children" for loop, the generic types are
not being added. I changed how the generic parameters are saved, so that now
any/all generic types can be saved instead of just one.

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:562: addClassChild(name, value, children);
On 2016/07/25 20:53:00, Alan Knight wrote:
> Good, but maybe for consistency with the clause below, do it more like
> 
>    children.add(classChild(name, value))

Done.

https://codereview.chromium.org/2176233002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/debugger.dart:571: genericClassHandler(String name,
Object genericTypeConstructor) {
On 2016/07/25 20:53:00, Alan Knight wrote:
> "Handler" is another one of those non-value-adding words. Also, better to name
> methods as verbs than nouns. Better to call this something like noteGenerics
or
> recordTypeParameters

Done.

[Jacob, 2016-07-26 00:40:42]

lgtm

[bmilligan, 2016-07-26 00:45:21]

Description was changed from

==========
Library children object fix
==========

to

==========
Library children object fix

R=alanknight@google.com, jacobr@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/ef03e00f4441fa8eb02fea0115b8...
==========

[bmilligan, 2016-07-26 00:45:23]

Committed patchset #5 (id:80001) manually as
ef03e00f4441fa8eb02fea0115b89d4be7e9f08e (presubmit successful).