-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Override == and hashCode in PbMap #224
Changes from 5 commits
103434e
fba29e8
94b9e5c
3f4e34e
e2d88d8
3663527
92389c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,40 @@ class PbMap<K, V> extends MapBase<K, V> { | |
_wrappedMap[key] = value; | ||
} | ||
|
||
/// A [PbMap] is equal to another [PbMap] with equal key/value | ||
/// pairs in any order. | ||
@override | ||
bool operator ==(other) { | ||
if (identical(other, this)) { | ||
return true; | ||
} | ||
if (other is! PbMap) { | ||
return false; | ||
} | ||
if (other.length != length) { | ||
return false; | ||
} | ||
for (final key in keys) { | ||
if (!other.containsKey(key)) { | ||
return false; | ||
} | ||
} | ||
for (final key in keys) { | ||
if (other[key] != this[key]) { | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
/// A [PbMap] is equal to another [PbMap] with equal key/value | ||
/// pairs in any order. Then, the `hashCode` is guaranteed to be the same. | ||
@override | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add dartdoc to pbmap explaining how the hash and equals semantics work. |
||
int get hashCode { | ||
sigurdm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return _wrappedMap.entries | ||
.fold(0, (h, entry) => h ^ _HashUtils._hash2(entry.key, entry.value)); | ||
} | ||
|
||
@override | ||
void clear() { | ||
if (_isReadonly) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,4 +35,32 @@ bool _areByteDataEqual(ByteData lhs, ByteData rhs) { | |
return _areListsEqual(asBytes(lhs), asBytes(rhs)); | ||
} | ||
|
||
List<T> sorted<T>(Iterable<T> list) => List.from(list)..sort(); | ||
@Deprecated("This function was not intended to be public. " | ||
"It will be removed from the public api in next major version. ") | ||
List<T> sorted<T>(Iterable<T> list) => new List.from(list)..sort(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is duplicate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's deprecate this, and make a private version. It has nothing to do in the exports of protobuf. |
||
|
||
List<T> _sorted<T>(Iterable<T> list) => new List.from(list)..sort(); | ||
|
||
class _HashUtils { | ||
// Jenkins hash functions copied from https://github.com/google/quiver-dart/blob/master/lib/src/core/hash.dart. | ||
|
||
static int _combine(int hash, int value) { | ||
hash = 0x1fffffff & (hash + value); | ||
hash = 0x1fffffff & (hash + ((0x0007ffff & hash) << 10)); | ||
return hash ^ (hash >> 6); | ||
} | ||
|
||
static int _finish(int hash) { | ||
hash = 0x1fffffff & (hash + ((0x03ffffff & hash) << 3)); | ||
hash = hash ^ (hash >> 11); | ||
return 0x1fffffff & (hash + ((0x00003fff & hash) << 15)); | ||
} | ||
|
||
/// Generates a hash code for multiple [objects]. | ||
static int _hashObjects(Iterable objects) => | ||
_finish(objects.fold(0, (h, i) => _combine(h, i.hashCode))); | ||
|
||
/// Generates a hash code for two objects. | ||
static int _hash2(a, b) => | ||
_finish(_combine(_combine(0, a.hashCode), b.hashCode)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ library map_field_test; | |
|
||
import 'dart:convert'; | ||
|
||
import 'package:protobuf/protobuf.dart'; | ||
import 'package:test/test.dart'; | ||
|
||
import '../out/protos/map_field.pb.dart'; | ||
|
@@ -212,6 +213,34 @@ void main() { | |
_expectEmpty(testMap); | ||
}); | ||
|
||
test( | ||
'PbMap` is equal to another PbMap with equal key/value pairs in any order', | ||
() { | ||
TestMap t = TestMap() | ||
..int32ToStringField[2] = 'test2' | ||
..int32ToStringField[1] = 'test'; | ||
TestMap t2 = TestMap() | ||
..int32ToStringField[1] = 'test' | ||
..int32ToStringField[2] = 'test2'; | ||
TestMap t3 = TestMap() | ||
..int32ToStringField[1] = 'test'; | ||
|
||
PbMap<int, String> m = t.int32ToStringField; | ||
PbMap<int, String> m2 = t2.int32ToStringField; | ||
PbMap<int, String> m3 = t3.int32ToStringField; | ||
|
||
expect(t, t2); | ||
expect(t.hashCode, t2.hashCode); | ||
|
||
expect(m, m2); | ||
expect(m == m2, isTrue); | ||
expect(m.hashCode, m2.hashCode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a negative test? |
||
|
||
expect(m, isNot(m3)); | ||
expect(m == m3, isFalse); | ||
expect(m.hashCode, isNot(m3.hashCode)); | ||
}); | ||
|
||
test('merge from other message', () { | ||
TestMap testMap = TestMap(); | ||
_setValues(testMap); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other[key]
could benull
.I'd add a comment that
this[key]
can never benull
, so it is not necessary to checkother.containsKey(key)
, since the test will fail for missingkey
.As a TODO - It might be more efficient to check all the keys first, since they are usually smaller than the values.