Skip to content
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

Merged
merged 7 commits into from
Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions protobuf/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.13.7

* Override `operator ==` and `hashCode` in `PbMap` so that two `PbMap`s are equal if they have equal key/value pairs.

## 0.13.5

* Add new method `addAll` on ExtensionRegistry for more conveniently adding multiple extensions at once.
Expand Down
2 changes: 1 addition & 1 deletion protobuf/lib/src/protobuf/coded_buffer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ void _writeToCodedBufferWriter(_FieldSet fs, CodedBufferWriter out) {
}

if (fs._hasExtensions) {
for (var tagNumber in sorted(fs._extensions._tagNumbers)) {
for (var tagNumber in _sorted(fs._extensions._tagNumbers)) {
var fi = fs._extensions._getInfoOrNull(tagNumber);
out.writeField(tagNumber, fi.type, fs._extensions._getFieldOrNull(fi));
}
Expand Down
53 changes: 25 additions & 28 deletions protobuf/lib/src/protobuf/field_set.dart
Original file line number Diff line number Diff line change
Expand Up @@ -523,51 +523,48 @@ class _FieldSet {
/// The hash may change when any field changes (recursively).
/// Therefore, protobufs used as map keys shouldn't be changed.
int get _hashCode {
int hash;

void hashEnumList(PbListBase enums) {
for (ProtobufEnum enm in enums) {
hash = 0x1fffffff & ((31 * hash) + enm.value);
}
}

// Hashes the value of one field (recursively).
void hashField(FieldInfo fi, value) {
int hashField(int hash, FieldInfo fi, value) {
if (value is List && value.isEmpty) {
return; // It's either repeated or an empty byte array.
return hash; // It's either repeated or an empty byte array.
}
hash = 0x1fffffff & ((37 * hash) + fi.tagNumber);

hash = _HashUtils._combine(hash, fi.tagNumber);
if (!_isEnum(fi.type)) {
hash = 0x1fffffff & ((53 * hash) + value.hashCode);
hash = _HashUtils._combine(hash, value.hashCode);
} else if (fi.isRepeated) {
hashEnumList(value);
hash = _HashUtils._hashObjects(value.map((enm) => enm.value));
} else {
ProtobufEnum enm = value;
hash = 0x1fffffff & ((53 * hash) + enm.value);
hash = _HashUtils._combine(hash, enm.value);
}

return hash;
}

void hashEachField() {
for (var fi in _infosSortedByTag) {
var v = _values[fi.index];
if (v != null) hashField(fi, v);
}
if (!_hasExtensions) return;
for (int tagNumber in sorted(_extensions._tagNumbers)) {
int hashEachField(int hash) {
//non-extension fields
hash = _infosSortedByTag.where((fi) => _values[fi.index] != null).fold(
hash, (int h, FieldInfo fi) => hashField(h, fi, _values[fi.index]));

if (!_hasExtensions) return hash;

hash =
_sorted(_extensions._tagNumbers).fold(hash, (int h, int tagNumber) {
var fi = _extensions._getInfoOrNull(tagNumber);
hashField(fi, _extensions._getFieldOrNull(fi));
}
return hashField(h, fi, _extensions._getFieldOrNull(fi));
});

return hash;
}

// Generate hash.
hash = 41;
// Hash with descriptor.
hash = 0x1fffffff & ((19 * hash) + _meta.hashCode);
int hash = _HashUtils._combine(0, _meta.hashCode);
// Hash with fields.
hashEachField();
hash = hashEachField(hash);
// Hash with unknown fields.
if (_hasUnknownFields) {
hash = 0x1fffffff & ((29 * hash) + _unknownFields.hashCode);
hash = _HashUtils._combine(hash, _unknownFields.hashCode);
}
return hash;
}
Expand Down
2 changes: 1 addition & 1 deletion protobuf/lib/src/protobuf/json.dart
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ Map<String, dynamic> _writeToJsonMap(_FieldSet fs) {
result['${fi.tagNumber}'] = convertToMap(value, fi.type);
}
if (fs._hasExtensions) {
for (int tagNumber in sorted(fs._extensions._tagNumbers)) {
for (int tagNumber in _sorted(fs._extensions._tagNumbers)) {
var value = fs._extensions._values[tagNumber];
if (value is List && value.isEmpty) {
continue; // It's repeated or an empty byte array.
Expand Down
13 changes: 2 additions & 11 deletions protobuf/lib/src/protobuf/pb_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -171,17 +171,8 @@ abstract class PbListBase<E> extends ListBase<E> {
bool operator ==(other) =>
(other is PbListBase) && _areListsEqual(other, this);

int get hashCode {
int hash = 0;
for (final value in _wrappedList) {
hash = 0x1fffffff & (hash + value.hashCode);
hash = 0x1fffffff & (hash + ((0x0007ffff & hash) << 10));
hash = hash ^ (hash >> 6);
}
hash = 0x1fffffff & (hash + ((0x03ffffff & hash) << 3));
hash = hash ^ (hash >> 11);
return 0x1fffffff & (hash + ((0x00003fff & hash) << 15));
}
@override
int get hashCode => _HashUtils._hashObjects(_wrappedList);

/// Returns an [Iterator] for the list.
Iterator<E> get iterator => _wrappedList.iterator;
Expand Down
34 changes: 34 additions & 0 deletions protobuf/lib/src/protobuf/pb_map.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other[key] could be null.
I'd add a comment that this[key] can never be null, so it is not necessary to check other.containsKey(key), since the test will fail for missing key.

As a TODO - It might be more efficient to check all the keys first, since they are usually smaller than the values.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 {
return _wrappedMap.entries
.fold(0, (h, entry) => h ^ _HashUtils._hash2(entry.key, entry.value));
}

@override
void clear() {
if (_isReadonly)
Expand Down
2 changes: 1 addition & 1 deletion protobuf/lib/src/protobuf/unknown_field_set.dart
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class UnknownFieldSet {
String _toString(String indent) {
var stringBuffer = StringBuffer();

for (int tag in sorted(_fields.keys)) {
for (int tag in _sorted(_fields.keys)) {
var field = _fields[tag];
for (var value in field.values) {
if (value is UnknownFieldSet) {
Expand Down
30 changes: 29 additions & 1 deletion protobuf/lib/src/protobuf/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicate

Copy link
Collaborator

Choose a reason for hiding this comment

The 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));
}
2 changes: 1 addition & 1 deletion protobuf/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: protobuf
version: 0.13.5
version: 0.13.7
author: Dart Team <misc@dartlang.org>
description: >
Runtime library for protocol buffers support.
Expand Down
1 change: 1 addition & 0 deletions protoc_plugin/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
## 16.0.3

* Sync Kythe metadata updates from internal repo:
* Remove the extra 1 (field name) from generated field paths,
so they refer to the whole field rather than the name.
Expand Down
4 changes: 2 additions & 2 deletions protoc_plugin/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: protoc_plugin
version: 16.0.3
version: 16.0.3-dev-2
author: Dart Team <misc@dartlang.org>
description: Protoc compiler plugin to generate Dart code
homepage: https://github.com/dart-lang/protobuf
Expand All @@ -10,7 +10,7 @@ environment:
dependencies:
fixnum: ^0.10.5
path: ^1.0.0
protobuf: ^0.13.4
protobuf: ^0.13.7
dart_style: ^1.0.6

dev_dependencies:
Expand Down
29 changes: 29 additions & 0 deletions protoc_plugin/test/map_field_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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);
Expand Down