Fix issues with protobuf equality comparisons

Fix issues with protobuf equality comparisons

- Reading a repeated field could change equality comparisons,
because repeated fields are initialized lazily.

- If a GeneratedMessage implements Map, we would compare it as
a map instead of as a GeneratedMessage.

BUG=https://github.com/dart-lang/dart-protobuf/issues/48
R=sgjesse@google.com

Committed: https://github.com/dart-lang/protobuf/commit/b301d29f82de0b69f293c83fba632bffb3d7d108

[skybrian, 2016-04-02 02:53:22]

skybrian@google.com changed reviewers:
+ sgjesse@google.com

[Søren Gjesse, 2016-04-04 16:06:27]

lgtm, but shouldn't there be some new test?

https://codereview.chromium.org/1852983002/diff/1/lib/src/protobuf/utils.dart
File lib/src/protobuf/utils.dart (right):

https://codereview.chromium.org/1852983002/diff/1/lib/src/protobuf/utils.dart...
lib/src/protobuf/utils.dart:10: if ((lhs is GeneratedMessage) && (rhs is
GeneratedMessage)) {
Maybe negate this if to only have one lhs == rhs.

if (!(lhs is Gener... {
  if ((lhs is List) ...
  if ((lhs is List) ...
  ...
}
return lhs == rhs;

[skybrian, 2016-04-04 19:37:32]

Description was changed from

==========
Fix issues with protobuf equality comparisons

- Reading a repeated field could change equality comparisons,
because repeated fields are initialized lazily.

- If a GeneratedMessage implements Map, we would compare it as
a map instead of as a GeneratedMessage.

BUG=https://github.com/dart-lang/dart-protobuf/issues/48
==========

to

==========
Fix issues with protobuf equality comparisons

- Reading a repeated field could change equality comparisons,
because repeated fields are initialized lazily.

- If a GeneratedMessage implements Map, we would compare it as
a map instead of as a GeneratedMessage.

BUG=https://github.com/dart-lang/dart-protobuf/issues/48
R=sgjesse@google.com

Committed:
https://github.com/dart-lang/protobuf/commit/b301d29f82de0b69f293c83fba632bff...
==========

[skybrian, 2016-04-04 19:37:33]

Committed patchset #2 (id:20001) manually as
b301d29f82de0b69f293c83fba632bffb3d7d108 (presubmit successful).

[skybrian, 2016-04-04 19:37:43]

On 2016/04/04 16:06:27, Søren Gjesse wrote:
> lgtm, but shouldn't there be some new test?

Not sure which tests you mean. There are new tests in test/map_mixin.dart. I
improved them to test hashCode as well, and added them to message_test so we
check the non-mixin case.

[skybrian, 2016-04-04 19:38:02]

https://codereview.chromium.org/1852983002/diff/1/lib/src/protobuf/utils.dart
File lib/src/protobuf/utils.dart (right):

https://codereview.chromium.org/1852983002/diff/1/lib/src/protobuf/utils.dart...
lib/src/protobuf/utils.dart:10: if ((lhs is GeneratedMessage) && (rhs is
GeneratedMessage)) {
On 2016/04/04 16:06:27, Søren Gjesse wrote:
> Maybe negate this if to only have one lhs == rhs.
> 
> if (!(lhs is Gener... {
>   if ((lhs is List) ...
>   if ((lhs is List) ...
>   ...
> }
> return lhs == rhs

As a style thing, I'd rather repeat the == check than have nested if statements.

I changed this code slightly to ensure that if only one or the other is a
GeneratedMessage, it will return false. But I don't think this will come up in
practice except in contrived situations.