Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2100803007:
Array formatting customized to look like JS (Closed)
|
Created:
4 years, 5 months ago by bmilligan Modified:
4 years, 5 months ago CC:
dev-compiler+reviews_dartlang.org Base URL:
https://github.com/dart-lang/dev_compiler.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionArray formatting customized to look like JS
BUG=
R=jacobr@google.com
Committed: https://github.com/dart-lang/dev_compiler/commit/16c26816235286e008b4cb9d779253e4d7fae222
Patch Set 1 #
Total comments: 26
Patch Set 2 : Logic and variable name updates #
Total comments: 16
Patch Set 3 : Naming and logic changes #Patch Set 4 : comments and 10001 case #Patch Set 5 : submit #
Messages
Total messages: 12 (3 generated)
Description was changed from ========== Merge branch 'master' of https://github.com/dart-lang/dev_compiler Array formatting customized to look like JS BUG= ========== to ========== Array formatting customized to look like JS BUG= ==========
bmilligan@google.com changed reviewers: + alanknight@google.com, jacobr@google.com
https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:102: NameValuePair({this.name, this.value, this.config: JsonMLConfig.none, this.hideName: false}); run dartfmt https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:114: String get displayName => hideName ? null : name; would '' be cleaner than null? In general prefer to avoid null values. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:127: final int low; instead of low and high use start, end instead of object use iterable https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:253: ..createTextChild(child.displayName != null ? child.displayName + ': ' : '') not your fault but change from child.displayName + ': ' to '${child.displayName}: ' https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:253: ..createTextChild(child.displayName != null ? child.displayName + ': ' : '') change to child.displayName.isEmpty ? '' : ... https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:511: ret.addAll(iterableChildren(new IterableSpan(low: 0, high: object.length-1, object: object))); no real need for method names to indicate the type of the arguments they take. Perhaps just call this childrenHelper I can't think of a good descriptive name. Perhaps Alan has a better idea. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:612: /// Formatter for synthetic MapEntry objects used to display contents of a Map This comment is wrong. Update it to be about your class. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:628: int range = span.high - span.low + 1; this should be called length not range. Also, no need for types on the lhs here and elsewhere in this method as they can be inferred. For consistency with other Dart apis, keep the spans exclusive rather than inclusive. thus as a span should be from span.start to span.end - 1 in which case int length = span.end - span.start https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:630: if (range <= 100) { make 100 a constant https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:631: for(int i = span.low; i < span.high+1; i++) { should be i < span.end https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:635: int a = (log(range-1)/log(100)).truncate(); what is a? can you write this code so it is clearer? at minimum this variable name should be easier to understand. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:636: for (int i = span.low; i < span.high; i += pow(100, a)) { nit: assign pow(100, a) to a variable rather than repeatedly computing it. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:637: int endIndex = min(span.high - i + 1, pow(100, a)) + i - 1; I expect by defining spans to be exclusive rather than inclusive you will simplify these expressions. I'll review them more carefully once the logic is updated..
https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:102: NameValuePair({this.name, this.value, this.config: JsonMLConfig.none, this.hideName: false}); On 2016/06/27 23:12:04, Jacob wrote: > run dartfmt Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:114: String get displayName => hideName ? null : name; On 2016/06/27 23:12:04, Jacob wrote: > would '' be cleaner than null? In general prefer to avoid null values. Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:127: final int low; On 2016/06/27 23:12:04, Jacob wrote: > instead of low and high > use > start, end > > instead of > object > use > iterable > Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:253: ..createTextChild(child.displayName != null ? child.displayName + ': ' : '') On 2016/06/27 23:12:04, Jacob wrote: > not your fault but change from > child.displayName + ': ' > to > '${child.displayName}: ' Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:253: ..createTextChild(child.displayName != null ? child.displayName + ': ' : '') On 2016/06/27 23:12:04, Jacob wrote: > change to > child.displayName.isEmpty ? '' : ... Should it be isNotEmpty? https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:511: ret.addAll(iterableChildren(new IterableSpan(low: 0, high: object.length-1, object: object))); On 2016/06/27 23:12:04, Jacob wrote: > no real need for method names to indicate the type of the arguments they take. > Perhaps just call this childrenHelper > I can't think of a good descriptive name. Perhaps Alan has a better idea. Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:612: /// Formatter for synthetic MapEntry objects used to display contents of a Map On 2016/06/27 23:12:04, Jacob wrote: > This comment is wrong. Update it to be about your class. Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:628: int range = span.high - span.low + 1; On 2016/06/27 23:12:04, Jacob wrote: > this should be called length not range. > Also, no need for types on the lhs here and elsewhere in this method as they can > be inferred. > For consistency with other Dart apis, keep the spans exclusive rather than > inclusive. thus as a span should be from > span.start to span.end - 1 > in which case > int length = span.end - span.start Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:630: if (range <= 100) { On 2016/06/27 23:12:04, Jacob wrote: > make 100 a constant Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:631: for(int i = span.low; i < span.high+1; i++) { On 2016/06/27 23:12:04, Jacob wrote: > should be > i < span.end Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:635: int a = (log(range-1)/log(100)).truncate(); On 2016/06/27 23:12:04, Jacob wrote: > what is a? can you write this code so it is clearer? > at minimum this variable name should be easier to understand. Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:636: for (int i = span.low; i < span.high; i += pow(100, a)) { On 2016/06/27 23:12:04, Jacob wrote: > nit: assign pow(100, a) to a variable rather than repeatedly computing it. Done. https://codereview.chromium.org/2100803007/diff/1/tool/input_sdk/private/debu... tool/input_sdk/private/debugger.dart:637: int endIndex = min(span.high - i + 1, pow(100, a)) + i - 1; On 2016/06/27 23:12:04, Jacob wrote: > I expect by defining spans to be exclusive rather than inclusive you will > simplify these expressions. I'll review them more carefully once the logic is > updated.. Done.
https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:636: if (length <= maxIterableChildrenSubset) { how about maxBlockLength or maxSpanLength instead of maxIterableChildrenSubset? https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:637: for (int i = span.start; i < span.end; i++) { var i instead of int i https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:639: name: i.toString(), value: span.iterable.elementAt(i))); Interesting algorithmic complexity case to consider: if an iterable happens to be a LinkedList or other iterable where elementAt is O(n) this could be quite slow. You could test how noticeable the slowdown is for a linked list with 10M elements. If the UI is still usable then don't worry about it. If the UI hangs, use iterable.skip and for (var element in iterable.skip(start)) { https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:643: (log(length - 1) / log(maxIterableChildrenSubset)).truncate(); why is this log(length - 1) instead of log(length)? length - 1 may be right, it should be documented. I imagine it is length - 1 so that 100 gets 1 still returns 0 instead of 1? https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:644: var subsize = pow(maxIterableChildrenSubset, maxPowerOfSubsetSize); camel case and avoid abbreviations for variable names. I recommend reading through https://www.dartlang.org/effective-dart/style/ https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:645: for (int i = span.start; i < span.end; i += subsize) { here and elsewhere, write var instead of int when types will be easily inferred. https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:646: var endIndex = min(span.end - i, subsize) + i; I think min(span.end, i + subsize) is clearer
https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:636: if (length <= maxIterableChildrenSubset) { On 2016/06/28 16:35:54, Jacob wrote: > how about > maxBlockLength > or > maxSpanLength > instead > of maxIterableChildrenSubset? Done. https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:637: for (int i = span.start; i < span.end; i++) { On 2016/06/28 16:35:54, Jacob wrote: > var i > instead of int i Done. https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:639: name: i.toString(), value: span.iterable.elementAt(i))); On 2016/06/28 16:35:54, Jacob wrote: > Interesting algorithmic complexity case to consider: if an iterable happens to > be a LinkedList or other iterable where elementAt is O(n) this could be quite > slow. > You could test how noticeable the slowdown is for a linked list with 10M > elements. If the UI is still usable then don't worry about it. If the UI hangs, > use iterable.skip and > for (var element in iterable.skip(start)) { I created a ListQueue of ~40M nodes and there was no delay in the UI for the custom formatters. I also tested using iterable.skip and that created lag in the UI, so I suppose I should stick with this algorithm. https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:643: (log(length - 1) / log(maxIterableChildrenSubset)).truncate(); On 2016/06/28 16:35:54, Jacob wrote: > why is this log(length - 1) instead of log(length)? > length - 1 may be right, it should be documented. > I imagine it is length - 1 so that 100 gets 1 still returns 0 instead of 1? Yes, the -1 is dealing with the perfect square case so that a list of length 10000 results in a maxPowerOfSubsetSize of 1 instead of 2, and therefore will be broken up into subsets of 100 instead of creating a loop of subsets of 10000 that never get broken up. https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:644: var subsize = pow(maxIterableChildrenSubset, maxPowerOfSubsetSize); On 2016/06/28 16:35:54, Jacob wrote: > camel case and avoid abbreviations for variable names. I recommend reading > through https://www.dartlang.org/effective-dart/style/ Done. https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:645: for (int i = span.start; i < span.end; i += subsize) { On 2016/06/28 16:35:54, Jacob wrote: > here and elsewhere, write > var > instead of int > when types will be easily inferred. Done. https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:646: var endIndex = min(span.end - i, subsize) + i; On 2016/06/28 16:35:54, Jacob wrote: > I think > min(span.end, i + subsize) > is clearer Done.
https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:639: name: i.toString(), value: span.iterable.elementAt(i))); On 2016/06/28 17:26:09, bmilligan wrote: > On 2016/06/28 16:35:54, Jacob wrote: > > Interesting algorithmic complexity case to consider: if an iterable happens to > > be a LinkedList or other iterable where elementAt is O(n) this could be quite > > slow. > > You could test how noticeable the slowdown is for a linked list with 10M > > elements. If the UI is still usable then don't worry about it. If the UI > hangs, > > use iterable.skip and > > for (var element in iterable.skip(start)) { > > I created a ListQueue of ~40M nodes and there was no delay in the UI for the > custom formatters. I also tested using iterable.skip and that created lag in the > UI, so I suppose I should stick with this algorithm. Go ahead and just add a TODO to stop using elementAt if it becomes a performance bottlekneck in the future.
https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... File tool/input_sdk/private/debugger.dart (right): https://codereview.chromium.org/2100803007/diff/20001/tool/input_sdk/private/... tool/input_sdk/private/debugger.dart:639: name: i.toString(), value: span.iterable.elementAt(i))); On 2016/06/28 17:37:02, Jacob wrote: > On 2016/06/28 17:26:09, bmilligan wrote: > > On 2016/06/28 16:35:54, Jacob wrote: > > > Interesting algorithmic complexity case to consider: if an iterable happens > to > > > be a LinkedList or other iterable where elementAt is O(n) this could be > quite > > > slow. > > > You could test how noticeable the slowdown is for a linked list with 10M > > > elements. If the UI is still usable then don't worry about it. If the UI > > hangs, > > > use iterable.skip and > > > for (var element in iterable.skip(start)) { > > > > I created a ListQueue of ~40M nodes and there was no delay in the UI for the > > custom formatters. I also tested using iterable.skip and that created lag in > the > > UI, so I suppose I should stick with this algorithm. > Go ahead and just add a TODO to stop using elementAt if it becomes a performance > bottlekneck in the future. Done.
lgtm
Description was changed from ========== Array formatting customized to look like JS BUG= ========== to ========== Array formatting customized to look like JS BUG= R=jacobr@google.com Committed: https://github.com/dart-lang/dev_compiler/commit/16c26816235286e008b4cb9d7792... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 16c26816235286e008b4cb9d779253e4d7fae222 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
