Add Equals() to mojom structs and related types.

Add Equals() to mojom structs and related types.

BUG=

Committed: https://crrev.com/6b5d1a9be85123b55109f92268a6bf31157f2b88
Cr-Commit-Position: refs/heads/master@{#299614}

[Aaron Boodman, 2014-10-14 21:10:52]

aa@chromium.org changed reviewers:
+ yzshen@chromium.org

[Aaron Boodman, 2014-10-14 21:25:44]

Patchset #3 (id:40001) has been deleted

[Aaron Boodman, 2014-10-14 21:25:51]

Patchset #2 (id:20001) has been deleted

[Aaron Boodman, 2014-10-14 21:25:56]

Patchset #1 (id:1) has been deleted

[yzshen1, 2014-10-14 21:47:21]

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/struct_ptr.h:77: return ptr_->Equals(*other.ptr_);
ptr_ and other.ptr_ may both be null here. (Please consider a test case for
this. :))

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/struct_ptr.h:151: return value_.Equals(other.value_);
It seems we don't need to compare the value if they are both null.

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/equals_unittest.cc (right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/equals_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
It seems currently we divide test cases by the object they test
(struct/array/map_unittests), instead of the functionality they test (e.g.,
serialization_unittest, clone_unittest). What do you think?

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
(right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:24:

nit: This results in two empty lines in the generated code. (But maybe we don't
care about the formatting of the resulting code?)

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:16:

(Results in two empty lines in the generated code)

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:32:
{%  for field in struct.fields %}
nit: {%- to avoid an unnecessary empty line.

[Aaron Boodman, 2014-10-14 22:30:13]

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/struct_ptr.h:77: return ptr_->Equals(*other.ptr_);
On 2014/10/14 21:47:21, yzshen1 wrote:
> ptr_ and other.ptr_ may both be null here. (Please consider a test case for
> this. :))

Good point! Thanks.

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/struct_ptr.h:151: return value_.Equals(other.value_);
On 2014/10/14 21:47:21, yzshen1 wrote:
> It seems we don't need to compare the value if they are both null.

Done.

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/equals_unittest.cc (right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/equals_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/10/14 21:47:21, yzshen1 wrote:
> It seems currently we divide test cases by the object they test
> (struct/array/map_unittests), instead of the functionality they test (e.g.,
> serialization_unittest, clone_unittest). What do you think?

In this case, it seemed less clean to do that because Equals() should be tested
across different types recursively.

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
(right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:24:

On 2014/10/14 21:47:21, yzshen1 wrote:
> nit: This results in two empty lines in the generated code. (But maybe we
don't
> care about the formatting of the resulting code?)

I see. That is unfortunate because it means we must choose between styling our
templates consistently with our normal source code, or having the output styled
consistently.

Since the purpose of style rules is to make code easier to understand, I think
we should choose to have the templates consistent with our other source code.

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:32:
{%  for field in struct.fields %}
On 2014/10/14 21:47:21, yzshen1 wrote:
> nit: {%- to avoid an unnecessary empty line. 

Done.

[yzshen1, 2014-10-14 22:47:40]

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/equals_unittest.cc (right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/equals_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Okay.

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
(right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:24:

On 2014/10/14 22:30:13, Aaron Boodman wrote:
> On 2014/10/14 21:47:21, yzshen1 wrote:
> > nit: This results in two empty lines in the generated code. (But maybe we
> don't
> > care about the formatting of the resulting code?)
> 
> I see. That is unfortunate because it means we must choose between styling our
> templates consistently with our normal source code, or having the output
styled
> consistently.
> 
> Since the purpose of style rules is to make code easier to understand, I think
> we should choose to have the templates consistent with our other source code.

I think the generated code is what the users want to consult (at least the .h
files).
This template code, on the other hand, is just read/touched by us the platform
developers.

Maybe we should make the template code look nice, and run clang-format on the
generated code? That makes both sides happy. :)

What do you think?

https://codereview.chromium.org/649633003/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/649633003/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:14: "equals_unittest.cc",
nit: please arrange alphabetically.

[Aaron Boodman, 2014-10-14 23:24:57]

I also fixed the duplicate template specializations in bindings_util.h

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
(right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:24:

On 2014/10/14 22:47:39, yzshen1 wrote:
> On 2014/10/14 22:30:13, Aaron Boodman wrote:
> > On 2014/10/14 21:47:21, yzshen1 wrote:
> > > nit: This results in two empty lines in the generated code. (But maybe we
> > don't
> > > care about the formatting of the resulting code?)
> > 
> > I see. That is unfortunate because it means we must choose between styling
our
> > templates consistently with our normal source code, or having the output
> styled
> > consistently.
> > 
> > Since the purpose of style rules is to make code easier to understand, I
think
> > we should choose to have the templates consistent with our other source
code.
> 
> I think the generated code is what the users want to consult (at least the .h
> files).
> This template code, on the other hand, is just read/touched by us the platform
> developers.
> 
> Maybe we should make the template code look nice, and run clang-format on the
> generated code? That makes both sides happy. :)
> 
> What do you think?

Does the SDK checkout guarantee clang is present?

https://codereview.chromium.org/649633003/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/649633003/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:14: "equals_unittest.cc",
On 2014/10/14 22:47:40, yzshen1 wrote:
> nit: please arrange alphabetically.

Done.

[yzshen1, 2014-10-14 23:33:37]

The clang format idea is just some random non-blocking suggestion.

Lgtm
On Oct 14, 2014 4:24 PM, <aa@chromium.org> wrote:

> I also fixed the duplicate template specializations in bindings_util.h
>
>
> https://codereview.chromium.org/649633003/diff/80001/mojo/
> public/tools/bindings/generators/cpp_templates/
> wrapper_class_declaration.tmpl
> File
> mojo/public/tools/bindings/generators/cpp_templates/
> wrapper_class_declaration.tmpl
> (right):
>
> https://codereview.chromium.org/649633003/diff/80001/mojo/
> public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.
> tmpl#newcode24
> mojo/public/tools/bindings/generators/cpp_templates/
> wrapper_class_declaration.tmpl:24:
>
> On 2014/10/14 22:47:39, yzshen1 wrote:
>
>> On 2014/10/14 22:30:13, Aaron Boodman wrote:
>> > On 2014/10/14 21:47:21, yzshen1 wrote:
>> > > nit: This results in two empty lines in the generated code. (But
>>
> maybe we
>
>> > don't
>> > > care about the formatting of the resulting code?)
>> >
>> > I see. That is unfortunate because it means we must choose between
>>
> styling our
>
>> > templates consistently with our normal source code, or having the
>>
> output
>
>> styled
>> > consistently.
>> >
>> > Since the purpose of style rules is to make code easier to
>>
> understand, I think
>
>> > we should choose to have the templates consistent with our other
>>
> source code.
>
>  I think the generated code is what the users want to consult (at least
>>
> the .h
>
>> files).
>> This template code, on the other hand, is just read/touched by us the
>>
> platform
>
>> developers.
>>
>
>  Maybe we should make the template code look nice, and run clang-format
>>
> on the
>
>> generated code? That makes both sides happy. :)
>>
>
>  What do you think?
>>
>
> Does the SDK checkout guarantee clang is present?
>
> https://codereview.chromium.org/649633003/diff/100001/
> mojo/public/cpp/bindings/tests/BUILD.gn
> File mojo/public/cpp/bindings/tests/BUILD.gn (right):
>
> https://codereview.chromium.org/649633003/diff/100001/
> mojo/public/cpp/bindings/tests/BUILD.gn#newcode14
> mojo/public/cpp/bindings/tests/BUILD.gn:14: "equals_unittest.cc",
> On 2014/10/14 22:47:40, yzshen1 wrote:
>
>> nit: please arrange alphabetically.
>>
>
> Done.
>
> https://codereview.chromium.org/649633003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-10-15 00:09:57]

lgtm

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
(right):

https://codereview.chromium.org/649633003/diff/80001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:24:

On 2014/10/14 23:24:57, Aaron Boodman wrote:
> On 2014/10/14 22:47:39, yzshen1 wrote:
> > On 2014/10/14 22:30:13, Aaron Boodman wrote:
> > > On 2014/10/14 21:47:21, yzshen1 wrote:
> > > > nit: This results in two empty lines in the generated code. (But maybe
we
> > > don't
> > > > care about the formatting of the resulting code?)
> > > 
> > > I see. That is unfortunate because it means we must choose between styling
> our
> > > templates consistently with our normal source code, or having the output
> > styled
> > > consistently.
> > > 
> > > Since the purpose of style rules is to make code easier to understand, I
> think
> > > we should choose to have the templates consistent with our other source
> code.
> > 
> > I think the generated code is what the users want to consult (at least the
.h
> > files).
> > This template code, on the other hand, is just read/touched by us the
platform
> > developers.
> > 
> > Maybe we should make the template code look nice, and run clang-format on
the
> > generated code? That makes both sides happy. :)
> > 
> > What do you think?
> 
> Does the SDK checkout guarantee clang is present?
I don't know. :)

[Aaron Boodman, 2014-10-15 00:27:06]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-10-15 00:29:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/649633003/160001

[commit-bot: I haz the power, 2014-10-15 01:18:15]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2014-10-15 01:18:50]

Patchset 6 (id:??) landed as
https://crrev.com/6b5d1a9be85123b55109f92268a6bf31157f2b88
Cr-Commit-Position: refs/heads/master@{#299614}