Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
370493002:
Fix utility function in smoke, add tests for it. (Closed)
|
Created:
6 years, 5 months ago by Siggi Cherem (dart-lang) Modified:
6 years, 5 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionFix utility function in smoke, add tests for it.
R=jakemac@google.com
Committed: https://code.google.com/p/dart/source/detail?r=38562
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Messages
Total messages: 13 (0 generated)
this is to address https://code.google.com/p/dart/issues/detail?id=19471
https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dart File pkg/smoke/lib/src/common.dart (right): https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dar... pkg/smoke/lib/src/common.dart:95: } I think that the code is still not right. [1 1 2 2 2] would still equal [1 1 1 2 2] (As a side note it would be more efficient to compare Set vs Set rather than compare Set vs List - we do build both aSet and bSet. However is is n/a as the code seems to be incorrect)
https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dart File pkg/smoke/lib/src/common.dart (right): https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dar... pkg/smoke/lib/src/common.dart:95: } On 2014/07/13 09:55:02, vicb wrote: > I think that the code is still not right. > > [1 1 2 2 2] would still equal [1 1 1 2 2] > > (As a side note it would be more efficient to compare Set vs Set rather than > compare Set vs List - we do build both aSet and bSet. However is is n/a as the > code seems to be incorrect) A Set is not going to work here, you need two maps of item->count. Calculate the counts for each list, then compare the counts.
On 2014/07/14 17:20:20, justinfagnani wrote: > https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dart > File pkg/smoke/lib/src/common.dart (right): > > https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dar... > pkg/smoke/lib/src/common.dart:95: } > On 2014/07/13 09:55:02, vicb wrote: > > I think that the code is still not right. > > > > [1 1 2 2 2] would still equal [1 1 1 2 2] > > > > (As a side note it would be more efficient to compare Set vs Set rather than > > compare Set vs List - we do build both aSet and bSet. However is is n/a as the > > code seems to be incorrect) > > A Set is not going to work here, you need two maps of item->count. Calculate the > counts for each list, then compare the counts. A slight optimization: One map of item->count. Calculate it for the first list, then decrement the counts for the second. If a.length == b.length, then you don't need to go into negative counts, just remove the key. The map should be empty if the lists are equal. If any count is ever greater than the remaining length of the list, then you can fail early, so keep a max count.
On 2014/07/14 17:23:31, justinfagnani wrote: > A slight optimization: One map of item->count. Calculate it for the first list, > then decrement the counts for the second. If a.length == b.length, then you > don't need to go into negative counts, just remove the key. The map should be > empty if the lists are equal. If any count is ever greater than the remaining > length of the list, then you can fail early, so keep a max count. Optimization should not be the pb here: unordered = true is used only for tests. What about dropping this arg and use UnorderedIterableEquality from dart:collection in tests ?
https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dart File pkg/smoke/lib/src/common.dart (right): https://codereview.chromium.org/370493002/diff/1/pkg/smoke/lib/src/common.dar... pkg/smoke/lib/src/common.dart:95: } On 2014/07/14 17:20:19, justinfagnani wrote: > On 2014/07/13 09:55:02, vicb wrote: > > I think that the code is still not right. > > > > [1 1 2 2 2] would still equal [1 1 1 2 2] > > > > (As a side note it would be more efficient to compare Set vs Set rather than > > compare Set vs List - we do build both aSet and bSet. However is is n/a as the > > code seems to be incorrect) > > A Set is not going to work here, you need two maps of item->count. Calculate the > counts for each list, then compare the counts. <facepalm> ... Ok I updated this again, hopefully this is better now. The reality is that this code is only used for the debug_static configuration, which is just used while developing code generators to verify that a static configuration contains everything you expect you'd get by comparing it with the results from using mirrors (ignoring the order of some operations). Most operations return lists, but in reality they are actually ordered-sets. So these issues with duplicates were never a concern there. We could rename this as compareListsContents and just use Set equality and we'd be good too.
lgtm https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/lib/src/common... File pkg/smoke/lib/src/common.dart (right): https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/lib/src/common... pkg/smoke/lib/src/common.dart:89: var count = countMap[x]; putIfAbsent?
https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/lib/src/common... File pkg/smoke/lib/src/common.dart (right): https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/lib/src/common... pkg/smoke/lib/src/common.dart:89: var count = countMap[x]; On 2014/07/23 20:13:44, jakemac wrote: > putIfAbsent? thanks for the suggestion - it could be countMap.putIfAbsent(x, () => 0); countMap[x]++; I am honestly equally happy with either this or the one I currently have, so I might just keep the one we have since it could be slightly cheaper.
On 2014/07/23 20:30:58, Siggi Cherem (dart-lang) wrote: > https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/lib/src/common... > File pkg/smoke/lib/src/common.dart (right): > > https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/lib/src/common... > pkg/smoke/lib/src/common.dart:89: var count = countMap[x]; > On 2014/07/23 20:13:44, jakemac wrote: > > putIfAbsent? > > thanks for the suggestion - it could be > > countMap.putIfAbsent(x, () => 0); > countMap[x]++; > > I am honestly equally happy with either this or the one I currently have, so I > might just keep the one we have since it could be slightly cheaper. Sounds fine to me, I the putIfAbsent version is just a tiny bit easier to read, but I don't feel strongly.
https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/test/common_ut... File pkg/smoke/test/common_utils_test.dart (right): https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/test/common_ut... pkg/smoke/test/common_utils_test.dart:34: expect(compareLists([1, 1, 2, 3, 4, 1], [2, 2, 1, 1, 3, 4], minor: duplicate test
https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/test/common_ut... File pkg/smoke/test/common_utils_test.dart (right): https://codereview.chromium.org/370493002/diff/40001/pkg/smoke/test/common_ut... pkg/smoke/test/common_utils_test.dart:34: expect(compareLists([1, 1, 2, 3, 4, 1], [2, 2, 1, 1, 3, 4], On 2014/07/23 21:23:53, vicb wrote: > minor: duplicate test Done.
Message was sent while issue was closed.
Committed patchset #3 manually as r38562 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
