mojom: Add associative arrays to the mojom language.

mojom: Add associative arrays to the mojom language.

This adds associative arrays to mojom to the main parser, and the C++ bindings generator. This adds a mojo::Map<> class to the C++ bindings, used by the generated bindings. Finally, it moves the clipboard.mojom interface over to associative arrays (which was the inspiration to add them to the mojom language).

BUG=413863
Committed: https://crrev.com/0f8647ec517acc9e80b0529fdb91fbd759ce85c3
Cr-Commit-Position: refs/heads/master@{#299017}

[Elliot Glaysher, 2014-09-26 22:08:44]

erg@chromium.org changed reviewers:
+ viettrungluu@chromium.org, yzshen@chromium.org

[Elliot Glaysher, 2014-09-26 22:08:45]

This is asking more for a feasibility review; this patch is a long way from
being landable.

I tried to get everything working previously by creating a Map_Data struct, and
trying to reuse the ArraySerializationHelper stuff. In the end, I couldn't reuse
or refactor the GetSerializationSize() methods so that they weren't directly
dependent on std::vector<>.

(I suppose I could have abandoned having move only classes as the value in maps,
but that felt unacceptable, because there's good reasons to have map<string,
vector<int> >, for example.)

So, do either of you see any problems with this approach? Or have any comments
about things I'll need to look out for?

Things that this patch doesn't cover yet:
- Serialization of structs. (In fact, what's there very much doesn't work.)
- Bindings for languages other than C++.

[yzshen1, 2014-09-27 00:01:58]

(I did a quick look.) This approach looks good to me.

It reduces complexity significantly. Although it has an extra step of creating
two Array objects during serialization/serialization, I am not too worried about
efficiency because complex objects are moved.

https://codereview.chromium.org/611633002/diff/1/mojo/public/cpp/bindings/str...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/611633002/diff/1/mojo/public/cpp/bindings/str...
mojo/public/cpp/bindings/string.h:43: String& operator=(const mojo::String& str)
{
Do you also want to add a copy constructor?

https://codereview.chromium.org/611633002/diff/1/mojo/services/public/interfa...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/611633002/diff/1/mojo/services/public/interfa...
mojo/services/public/interfaces/clipboard/clipboard.mojom:51: FakeTestMethod()
=> (string[string] mapping);
I think the constraint is that only primitive types and string are allowed to be
key, right? In the case of string, do you allow null as key? (I guess not.)

[viettrungluu, 2014-09-29 19:34:48]

https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):

https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
mojo/public/tools/bindings/pylib/mojom/parse/parser.py:240: | associated_array
"associated" -> "associative"

https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280: """associated_array
: typename LBRACKET basictypename RBRACKET"""
a) It seems dubious/incorrect to allow "handletypes" as keys.
b) I'm not sure about the syntax, but note that the grammar is dubious and
parses things "backwards"[*], which is unfortunate. (This is an existing
problem, even with arrays.)
c) Allowing even arbitrary identifiers but not handletypes for keys is probably
not what you want. This allows structs as keys, which may then contain handles.
(Of course, not allowing arbitrary-ish things may preclude allowing strings.)

[*] E.g., the type of "string foo[int32][double];" is parsed as double -> (int32
-> string), whereas you probably want it to mean int32 -> (double -> string).

[yzshen1, 2014-09-29 19:53:24]

https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):

https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280: """associated_array
: typename LBRACKET basictypename RBRACKET"""
On 2014/09/29 19:34:47, viettrungluu wrote:
> a) It seems dubious/incorrect to allow "handletypes" as keys.
> b) I'm not sure about the syntax, but note that the grammar is dubious and
> parses things "backwards"[*], which is unfortunate. (This is an existing
> problem, even with arrays.)
> c) Allowing even arbitrary identifiers but not handletypes for keys is
probably
> not what you want. This allows structs as keys, which may then contain
handles.
> (Of course, not allowing arbitrary-ish things may preclude allowing strings.)
> 
> [*] E.g., the type of "string foo[int32][double];" is parsed as double ->
(int32
> -> string), whereas you probably want it to mean int32 -> (double -> string).


About the "backward" issue, I am not sure whether changing it will make other
things less clear. For example:

string[int][handle]?

The trailing '?' means "the whole thing is nullable", which seems natural to
associate with the outermost array.

[viettrungluu, 2014-10-02 02:34:51]

On 2014/09/29 19:53:24, yzshen1 wrote:
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):
> 
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280:
"""associated_array
> : typename LBRACKET basictypename RBRACKET"""
> On 2014/09/29 19:34:47, viettrungluu wrote:
> > a) It seems dubious/incorrect to allow "handletypes" as keys.
> > b) I'm not sure about the syntax, but note that the grammar is dubious and
> > parses things "backwards"[*], which is unfortunate. (This is an existing
> > problem, even with arrays.)
> > c) Allowing even arbitrary identifiers but not handletypes for keys is
> probably
> > not what you want. This allows structs as keys, which may then contain
> handles.
> > (Of course, not allowing arbitrary-ish things may preclude allowing
strings.)
> > 
> > [*] E.g., the type of "string foo[int32][double];" is parsed as double ->
> (int32
> > -> string), whereas you probably want it to mean int32 -> (double ->
string).
> 
> 
> About the "backward" issue, I am not sure whether changing it will make other
> things less clear. For example:
> 
> string[int][handle]?
> 
> The trailing '?' means "the whole thing is nullable", which seems natural to
> associate with the outermost array.

Uhm, so you think string[5][10] should mean that the outer array has dimension
10?

[yzshen1, 2014-10-02 05:15:13]

On 2014/10/02 02:34:51, viettrungluu wrote:
> On 2014/09/29 19:53:24, yzshen1 wrote:
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):
> > 
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280:
> """associated_array
> > : typename LBRACKET basictypename RBRACKET"""
> > On 2014/09/29 19:34:47, viettrungluu wrote:
> > > a) It seems dubious/incorrect to allow "handletypes" as keys.
> > > b) I'm not sure about the syntax, but note that the grammar is dubious and
> > > parses things "backwards"[*], which is unfortunate. (This is an existing
> > > problem, even with arrays.)
> > > c) Allowing even arbitrary identifiers but not handletypes for keys is
> > probably
> > > not what you want. This allows structs as keys, which may then contain
> > handles.
> > > (Of course, not allowing arbitrary-ish things may preclude allowing
> strings.)
> > > 
> > > [*] E.g., the type of "string foo[int32][double];" is parsed as double ->
> > (int32
> > > -> string), whereas you probably want it to mean int32 -> (double ->
> string).
> > 
> > 
> > About the "backward" issue, I am not sure whether changing it will make
other
> > things less clear. For example:
> > 
> > string[int][handle]?
> > 
> > The trailing '?' means "the whole thing is nullable", which seems natural to
> > associate with the outermost array.
> 
> Uhm, so you think string[5][10] should mean that the outer array has dimension
> 10?

(I know.... Last time when we discussed this case, I voted for [10] referring to
the inner array.)

I think I am not 100% happy with either of the solution. :/
- If string[5][10] means the outer array has dimension 5, then it is not clear
what '?' means:
  string?[5][10] -> the elements of the inner array (whose type is string) can
be null.
  string[5]?[10] -> because [5] refers to the outer array, does this mean the
whole thing is nullable? It seems confusing.
  string[5][10]? -> the elements of the outer array (whose type is string[10])
can be null.

- If string[5][10] means the outer array has dimension 10, the ordering of
dimensions may seem confusing. But it is more clear what '?' at various
positions means:
  string?[5][10] -> the elements of the inner array (whose type is string) can
be null.
  string[5]?[10] -> the elements of the outer array (whose type is string[5])
can be null.
  string[5][10]? -> the whole thing is nullable.
  In other words, '?' always means that "the part to the left of '?'" is
nullable.

[viettrungluu, 2014-10-02 06:10:24]

On 2014/10/02 05:15:13, yzshen1 wrote:
> On 2014/10/02 02:34:51, viettrungluu wrote:
> > On 2014/09/29 19:53:24, yzshen1 wrote:
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280:
> > """associated_array
> > > : typename LBRACKET basictypename RBRACKET"""
> > > On 2014/09/29 19:34:47, viettrungluu wrote:
> > > > a) It seems dubious/incorrect to allow "handletypes" as keys.
> > > > b) I'm not sure about the syntax, but note that the grammar is dubious
and
> > > > parses things "backwards"[*], which is unfortunate. (This is an existing
> > > > problem, even with arrays.)
> > > > c) Allowing even arbitrary identifiers but not handletypes for keys is
> > > probably
> > > > not what you want. This allows structs as keys, which may then contain
> > > handles.
> > > > (Of course, not allowing arbitrary-ish things may preclude allowing
> > strings.)
> > > > 
> > > > [*] E.g., the type of "string foo[int32][double];" is parsed as double
->
> > > (int32
> > > > -> string), whereas you probably want it to mean int32 -> (double ->
> > string).
> > > 
> > > 
> > > About the "backward" issue, I am not sure whether changing it will make
> other
> > > things less clear. For example:
> > > 
> > > string[int][handle]?
> > > 
> > > The trailing '?' means "the whole thing is nullable", which seems natural
to
> > > associate with the outermost array.
> > 
> > Uhm, so you think string[5][10] should mean that the outer array has
dimension
> > 10?
> 
> (I know.... Last time when we discussed this case, I voted for [10] referring
to
> the inner array.)
> 
> I think I am not 100% happy with either of the solution. :/
> - If string[5][10] means the outer array has dimension 5, then it is not clear
> what '?' means:
>   string?[5][10] -> the elements of the inner array (whose type is string) can
> be null.
>   string[5]?[10] -> because [5] refers to the outer array, does this mean the
> whole thing is nullable? It seems confusing.

This seems like the only sensible interpretation, and perfectly natural to me.

>   string[5][10]? -> the elements of the outer array (whose type is string[10])
> can be null.
> 
> - If string[5][10] means the outer array has dimension 10, the ordering of
> dimensions may seem confusing.

I can guarantee you that no one in the world thinks string[5][10] means that,
even if logically it'd be nice if string[5][10] meant (string[5])[10].

So what you say below is moot.

> But it is more clear what '?' at various
> positions means:
>   string?[5][10] -> the elements of the inner array (whose type is string) can
> be null.
>   string[5]?[10] -> the elements of the outer array (whose type is string[5])
> can be null.
>   string[5][10]? -> the whole thing is nullable.
>   In other words, '?' always means that "the part to the left of '?'" is
> nullable.

[viettrungluu, 2014-10-02 07:04:53]

On 2014/10/02 06:10:24, viettrungluu wrote:
> On 2014/10/02 05:15:13, yzshen1 wrote:
> > On 2014/10/02 02:34:51, viettrungluu wrote:
> > > On 2014/09/29 19:53:24, yzshen1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > > File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > > mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280:
> > > """associated_array
> > > > : typename LBRACKET basictypename RBRACKET"""
> > > > On 2014/09/29 19:34:47, viettrungluu wrote:
> > > > > a) It seems dubious/incorrect to allow "handletypes" as keys.
> > > > > b) I'm not sure about the syntax, but note that the grammar is dubious
> and
> > > > > parses things "backwards"[*], which is unfortunate. (This is an
existing
> > > > > problem, even with arrays.)
> > > > > c) Allowing even arbitrary identifiers but not handletypes for keys is
> > > > probably
> > > > > not what you want. This allows structs as keys, which may then contain
> > > > handles.
> > > > > (Of course, not allowing arbitrary-ish things may preclude allowing
> > > strings.)
> > > > > 
> > > > > [*] E.g., the type of "string foo[int32][double];" is parsed as double
> ->
> > > > (int32
> > > > > -> string), whereas you probably want it to mean int32 -> (double ->
> > > string).
> > > > 
> > > > 
> > > > About the "backward" issue, I am not sure whether changing it will make
> > other
> > > > things less clear. For example:
> > > > 
> > > > string[int][handle]?
> > > > 
> > > > The trailing '?' means "the whole thing is nullable", which seems
natural
> to
> > > > associate with the outermost array.
> > > 
> > > Uhm, so you think string[5][10] should mean that the outer array has
> dimension
> > > 10?
> > 
> > (I know.... Last time when we discussed this case, I voted for [10]
referring
> to
> > the inner array.)
> > 
> > I think I am not 100% happy with either of the solution. :/
> > - If string[5][10] means the outer array has dimension 5, then it is not
clear
> > what '?' means:
> >   string?[5][10] -> the elements of the inner array (whose type is string)
can
> > be null.
> >   string[5]?[10] -> because [5] refers to the outer array, does this mean
the
> > whole thing is nullable? It seems confusing.
> 
> This seems like the only sensible interpretation, and perfectly natural to me.
> 
> >   string[5][10]? -> the elements of the outer array (whose type is
string[10])
> > can be null.
> > 
> > - If string[5][10] means the outer array has dimension 10, the ordering of
> > dimensions may seem confusing.
> 
> I can guarantee you that no one in the world thinks string[5][10] means that,
> even if logically it'd be nice if string[5][10] meant (string[5])[10].
> 
> So what you say below is moot.
> 
> > But it is more clear what '?' at various
> > positions means:
> >   string?[5][10] -> the elements of the inner array (whose type is string)
can
> > be null.
> >   string[5]?[10] -> the elements of the outer array (whose type is
string[5])
> > can be null.
> >   string[5][10]? -> the whole thing is nullable.
> >   In other words, '?' always means that "the part to the left of '?'" is
> > nullable.

To be a little more clear, the only reason for having the C/C++/Java/etc.-like
syntax |string[5][10]| is familiarity.

C-style type syntax is otherwise basically terrible (just be glad we don't have
pointers, function types, etc.).

The good news is that the common cases like |string[]?|, |string[5]?|,
|string[5][10]| do what people think they do.

The only reasonable alternative is to go with unambiguous syntax (like in erg's
Patch Set 4 description):

array<string>
array<string, 5>
array<array<string, 10>, 5>
array<string>?
array<string?>
array<array<string, 10>?, 5>
array<array<string, 10>, 5>?
array<array<string, 10>?, 5>?

This has the advantage that it's obvious to everyone what the types in question
are. It has the disadvantage that the common cases become rather verbose.

(Of course, we've done something terrible with our "interface request" notation,
using '&'. Quite possibly we should do away with that too, and go with something
a little more clear there too.)

[yzshen1, 2014-10-02 07:37:58]

On 2014/10/02 07:04:53, viettrungluu wrote:
> On 2014/10/02 06:10:24, viettrungluu wrote:
> > On 2014/10/02 05:15:13, yzshen1 wrote:
> > > On 2014/10/02 02:34:51, viettrungluu wrote:
> > > > On 2014/09/29 19:53:24, yzshen1 wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > > > File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > > > mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280:
> > > > """associated_array
> > > > > : typename LBRACKET basictypename RBRACKET"""
> > > > > On 2014/09/29 19:34:47, viettrungluu wrote:
> > > > > > a) It seems dubious/incorrect to allow "handletypes" as keys.
> > > > > > b) I'm not sure about the syntax, but note that the grammar is
dubious
> > and
> > > > > > parses things "backwards"[*], which is unfortunate. (This is an
> existing
> > > > > > problem, even with arrays.)
> > > > > > c) Allowing even arbitrary identifiers but not handletypes for keys
is
> > > > > probably
> > > > > > not what you want. This allows structs as keys, which may then
contain
> > > > > handles.
> > > > > > (Of course, not allowing arbitrary-ish things may preclude allowing
> > > > strings.)
> > > > > > 
> > > > > > [*] E.g., the type of "string foo[int32][double];" is parsed as
double
> > ->
> > > > > (int32
> > > > > > -> string), whereas you probably want it to mean int32 -> (double ->
> > > > string).
> > > > > 
> > > > > 
> > > > > About the "backward" issue, I am not sure whether changing it will
make
> > > other
> > > > > things less clear. For example:
> > > > > 
> > > > > string[int][handle]?
> > > > > 
> > > > > The trailing '?' means "the whole thing is nullable", which seems
> natural
> > to
> > > > > associate with the outermost array.
> > > > 
> > > > Uhm, so you think string[5][10] should mean that the outer array has
> > dimension
> > > > 10?
> > > 
> > > (I know.... Last time when we discussed this case, I voted for [10]
> referring
> > to
> > > the inner array.)
> > > 
> > > I think I am not 100% happy with either of the solution. :/
> > > - If string[5][10] means the outer array has dimension 5, then it is not
> clear
> > > what '?' means:
> > >   string?[5][10] -> the elements of the inner array (whose type is string)
> can
> > > be null.
> > >   string[5]?[10] -> because [5] refers to the outer array, does this mean
> the
> > > whole thing is nullable? It seems confusing.
> > 
> > This seems like the only sensible interpretation, and perfectly natural to
me.
> > 
> > >   string[5][10]? -> the elements of the outer array (whose type is
> string[10])
> > > can be null.
> > > 
> > > - If string[5][10] means the outer array has dimension 10, the ordering of
> > > dimensions may seem confusing.
> > 
> > I can guarantee you that no one in the world thinks string[5][10] means
that,
> > even if logically it'd be nice if string[5][10] meant (string[5])[10].
> > 
> > So what you say below is moot.
> > 
> > > But it is more clear what '?' at various
> > > positions means:
> > >   string?[5][10] -> the elements of the inner array (whose type is string)
> can
> > > be null.
> > >   string[5]?[10] -> the elements of the outer array (whose type is
> string[5])
> > > can be null.
> > >   string[5][10]? -> the whole thing is nullable.
> > >   In other words, '?' always means that "the part to the left of '?'" is
> > > nullable.
> 
> To be a little more clear, the only reason for having the C/C++/Java/etc.-like
> syntax |string[5][10]| is familiarity.
> 
> C-style type syntax is otherwise basically terrible (just be glad we don't
have
> pointers, function types, etc.).
> 
> The good news is that the common cases like |string[]?|, |string[5]?|,
> |string[5][10]| do what people think they do.
> 
> The only reasonable alternative is to go with unambiguous syntax (like in
erg's
> Patch Set 4 description):
> 
> array<string>
> array<string, 5>
> array<array<string, 10>, 5>
> array<string>?
> array<string?>
> array<array<string, 10>?, 5>
> array<array<string, 10>, 5>?
> array<array<string, 10>?, 5>?
> 
> This has the advantage that it's obvious to everyone what the types in
question
> are. It has the disadvantage that the common cases become rather verbose.
> 
> (Of course, we've done something terrible with our "interface request"
notation,
> using '&'. Quite possibly we should do away with that too, and go with
something
> a little more clear there too.)

Haha, I came back to this thread thinking that I would propose something that
you just did! :)
(The only difference is that I was thinking maybe we could use "array<4 int>" to
indicate a fixed array.)

I don't think that people will likely understand what "string[]?[int][4]" means,
no matter what ordering of dimensions we use.
I prefer the verbose notation: "array<map<int, array<4 string>>>?". Although it
is much longer, I think most people can understand without consulting
documentation / doing translation in their mind.

And actually this notation seems quite consistent with things such as
handle<data_pipe_consumer>.

[yzshen1, 2014-10-02 07:47:34]

On 2014/10/02 07:37:58, yzshen1 wrote:
> On 2014/10/02 07:04:53, viettrungluu wrote:
> > On 2014/10/02 06:10:24, viettrungluu wrote:
> > > On 2014/10/02 05:15:13, yzshen1 wrote:
> > > > On 2014/10/02 02:34:51, viettrungluu wrote:
> > > > > On 2014/09/29 19:53:24, yzshen1 wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > > > > File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
> > > > > > mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280:
> > > > > """associated_array
> > > > > > : typename LBRACKET basictypename RBRACKET"""
> > > > > > On 2014/09/29 19:34:47, viettrungluu wrote:
> > > > > > > a) It seems dubious/incorrect to allow "handletypes" as keys.
> > > > > > > b) I'm not sure about the syntax, but note that the grammar is
> dubious
> > > and
> > > > > > > parses things "backwards"[*], which is unfortunate. (This is an
> > existing
> > > > > > > problem, even with arrays.)
> > > > > > > c) Allowing even arbitrary identifiers but not handletypes for
keys
> is
> > > > > > probably
> > > > > > > not what you want. This allows structs as keys, which may then
> contain
> > > > > > handles.
> > > > > > > (Of course, not allowing arbitrary-ish things may preclude
allowing
> > > > > strings.)
> > > > > > > 
> > > > > > > [*] E.g., the type of "string foo[int32][double];" is parsed as
> double
> > > ->
> > > > > > (int32
> > > > > > > -> string), whereas you probably want it to mean int32 -> (double
->
> > > > > string).
> > > > > > 
> > > > > > 
> > > > > > About the "backward" issue, I am not sure whether changing it will
> make
> > > > other
> > > > > > things less clear. For example:
> > > > > > 
> > > > > > string[int][handle]?
> > > > > > 
> > > > > > The trailing '?' means "the whole thing is nullable", which seems
> > natural
> > > to
> > > > > > associate with the outermost array.
> > > > > 
> > > > > Uhm, so you think string[5][10] should mean that the outer array has
> > > dimension
> > > > > 10?
> > > > 
> > > > (I know.... Last time when we discussed this case, I voted for [10]
> > referring
> > > to
> > > > the inner array.)
> > > > 
> > > > I think I am not 100% happy with either of the solution. :/
> > > > - If string[5][10] means the outer array has dimension 5, then it is not
> > clear
> > > > what '?' means:
> > > >   string?[5][10] -> the elements of the inner array (whose type is
string)
> > can
> > > > be null.
> > > >   string[5]?[10] -> because [5] refers to the outer array, does this
mean
> > the
> > > > whole thing is nullable? It seems confusing.
> > > 
> > > This seems like the only sensible interpretation, and perfectly natural to
> me.
> > > 
> > > >   string[5][10]? -> the elements of the outer array (whose type is
> > string[10])
> > > > can be null.
> > > > 
> > > > - If string[5][10] means the outer array has dimension 10, the ordering
of
> > > > dimensions may seem confusing.
> > > 
> > > I can guarantee you that no one in the world thinks string[5][10] means
> that,
> > > even if logically it'd be nice if string[5][10] meant (string[5])[10].
> > > 
> > > So what you say below is moot.
> > > 
> > > > But it is more clear what '?' at various
> > > > positions means:
> > > >   string?[5][10] -> the elements of the inner array (whose type is
string)
> > can
> > > > be null.
> > > >   string[5]?[10] -> the elements of the outer array (whose type is
> > string[5])
> > > > can be null.
> > > >   string[5][10]? -> the whole thing is nullable.
> > > >   In other words, '?' always means that "the part to the left of '?'" is
> > > > nullable.
> > 
> > To be a little more clear, the only reason for having the
C/C++/Java/etc.-like
> > syntax |string[5][10]| is familiarity.
> > 
> > C-style type syntax is otherwise basically terrible (just be glad we don't
> have
> > pointers, function types, etc.).
> > 
> > The good news is that the common cases like |string[]?|, |string[5]?|,
> > |string[5][10]| do what people think they do.
> > 
> > The only reasonable alternative is to go with unambiguous syntax (like in
> erg's
> > Patch Set 4 description):
> > 
> > array<string>
> > array<string, 5>
> > array<array<string, 10>, 5>
> > array<string>?
> > array<string?>
> > array<array<string, 10>?, 5>
> > array<array<string, 10>, 5>?
> > array<array<string, 10>?, 5>?
> > 
> > This has the advantage that it's obvious to everyone what the types in
> question
> > are. It has the disadvantage that the common cases become rather verbose.
> > 
> > (Of course, we've done something terrible with our "interface request"
> notation,
> > using '&'. Quite possibly we should do away with that too, and go with
> something
> > a little more clear there too.)
> 
> Haha, I came back to this thread thinking that I would propose something that
> you just did! :)
> (The only difference is that I was thinking maybe we could use "array<4 int>"
to
> indicate a fixed array.)
> 
> I don't think that people will likely understand what "string[]?[int][4]"
means,
> no matter what ordering of dimensions we use.
> I prefer the verbose notation: "array<map<int, array<4 string>>>?". Although
it
> is much longer, I think most people can understand without consulting
> documentation / doing translation in their mind.
> 
> And actually this notation seems quite consistent with things such as
> handle<data_pipe_consumer>.

Some more thoughts:
Because this syntax is only used in mojom files, I don't think being a little
verbose is a big cost.
It is nice to be self-documented in the interface definition.

And we are using (almost) the same syntax in the C++ bindings:
  array<map<int, array<4 string>> maps to Array<Map<int, Array<String>>>
, which also seems nice to me. :)

[Elliot Glaysher, 2014-10-02 19:54:02]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2014-10-02 19:54:04]

OK, this is now ready for a wider audience. Please focus more on the C++ code
this time.

(The plan, so far, is to implement the array<> syntax before landing this, and
to make the relatively small map<> syntax changes before landing this.)

https://codereview.chromium.org/611633002/diff/1/mojo/public/cpp/bindings/str...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/611633002/diff/1/mojo/public/cpp/bindings/str...
mojo/public/cpp/bindings/string.h:43: String& operator=(const mojo::String& str)
{
On 2014/09/27 00:01:58, yzshen1 wrote:
> Do you also want to add a copy constructor?

Done.

https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
File mojo/public/tools/bindings/pylib/mojom/parse/parser.py (right):

https://codereview.chromium.org/611633002/diff/1/mojo/public/tools/bindings/p...
mojo/public/tools/bindings/pylib/mojom/parse/parser.py:280: """associated_array
: typename LBRACKET basictypename RBRACKET"""
On 2014/09/29 19:34:47, viettrungluu wrote:
> a) It seems dubious/incorrect to allow "handletypes" as keys.

Fixed by changing the grammar to look for "identifier".

> b) I'm not sure about the syntax, but note that the grammar is dubious and
> parses things "backwards"[*], which is unfortunate. (This is an existing
> problem, even with arrays.)

This is large and thorny enough that I'm not going to deal with it in the
general case. I've fixed this in translate.py for the specific case of
associative arrays.

The actual fix to this is "rewrite the mojom compiler".

> c) Allowing even arbitrary identifiers but not handletypes for keys is
probably
> not what you want. This allows structs as keys, which may then contain
handles.
> (Of course, not allowing arbitrary-ish things may preclude allowing strings.)

Fixed this by adding type checks at a later phase once we have typing
information.

https://codereview.chromium.org/611633002/diff/1/mojo/services/public/interfa...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/611633002/diff/1/mojo/services/public/interfa...
mojo/services/public/interfaces/clipboard/clipboard.mojom:51: FakeTestMethod()
=> (string[string] mapping);
On 2014/09/27 00:01:58, yzshen1 wrote:
> I think the constraint is that only primitive types and string are allowed to
be
> key, right? In the case of string, do you allow null as key? (I guess not.)

Correct. We verify that we don't pass "string?" at the grammar level. (And then
rely on Array<String> checking the nullness of the incoming strings during
validation.)

[yzshen1, 2014-10-03 18:03:38]

First part of comments.

Thanks!

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_internal.h (right):

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:18:
MOJO_COMPILE_ASSERT(!internal::IsMoveOnlyType<Key>::value,
(optional) you can use the corresponding C++11 feature now, here and elsewhere.

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:53: static inline ValueRefType
at(std::map<KeyStorageType, ValueStorageType>* m,
Does it also need a const version?
ValueConstRefType at(const std::map<blah>* ...)

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:63:
ValueStorageType>::const_iterator& it) {
(optional) I feel that it is easy to read if ValueStorageType is aligned with
KeyStorageType on the previous line (here and elsewhere).

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:15: bool move_only =
IsMoveOnlyType<MapType>::value>
nit: the format kMoveOnly is used in map_internal. it is nice to be consistent.

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:98: inline size_t
GetSerializedSize_(const Map<MapKey, MapValue>& input) {
I probably haven't understood:
I thought it is okay to just compute GetSerializeSize_(KeyArray) +
GetSerializeSize_(MapArray).

Although the |num_bytes| field of an array is not guaranteed to be
8-byte-multiple, calling GetSerializeSize_() on arrays always returns
8-byte-multiple results. Therefore, I think that is sufficient. Have I missed
anything?

(The rationale is that |num_bytes| indicates the actual bytes used by an array,
while GetSerializeSize_() is the bytes reserved, making it 8-byte-multiple
guarantees that the next object starts at 8-byte boundaries.)

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map.h (right):

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:14: // Provides read-only access to map data.
What do you mean by it is read-only?

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:75: void insert(KeyForwardType key,
ValueForwardType value) {
Could you please comment about the behavior when |key| already exists in the
map?

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:80: ValueRefType at(KeyForwardType key) { return
Traits::at(&map_, key); }
I think it is good to have a const version.

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:171: class MapTypeConverterCallback {
What is this for? I didn't see any usage of it.

https://codereview.chromium.org/611633002/diff/100001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/611633002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:1: {%-
macro validate_field(struct, name, kind) %}
Please validate that both arrays for a map are of the same size.
(And we need to add validation tests, and maybe new validation error enum value,
for it. It is fine if you would like to do this part in a separate CL.)

[yzshen1, 2014-10-03 18:13:48]

One more comment:

I think the current patch hasn't dealt with the serialization/etc. work for
Array<Map<X, Y>>.

In this case, each array element consists of two array pointers, which is
something that the current logic in array_serialization.h hasn't handled.

[Elliot Glaysher, 2014-10-03 21:11:49]

> I think the current patch hasn't dealt with the serialization/etc. work for
Array<Map<X, Y>>.

Other than putting a check to make sure that we don't have an array of maps, do
you have any recommendations on how to deal with that?

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:98: inline size_t
GetSerializedSize_(const Map<MapKey, MapValue>& input) {
On 2014/10/03 18:03:38, yzshen1 wrote:
> I probably haven't understood:
> I thought it is okay to just compute GetSerializeSize_(KeyArray) +
> GetSerializeSize_(MapArray).

The problem is that there's no such thing as KeyArray or MapArray at this point.
We have been passed this Map from a struct to get the size that it takes. We
can't decompose the map because it's been given to us const. We probably don't
want to Clone it because that requires the Value type to be Clone()able. (Also,
cloning arbitrarily large data structures just to count the size is the Wrong
Thing.)

Note that this code is only called when you have a Map as a member of a struct.

> Although the |num_bytes| field of an array is not guaranteed to be
> 8-byte-multiple, calling GetSerializeSize_() on arrays always returns
> 8-byte-multiple results. Therefore, I think that is sufficient. Have I missed
> anything?
> 
> (The rationale is that |num_bytes| indicates the actual bytes used by an
array,
> while GetSerializeSize_() is the bytes reserved, making it 8-byte-multiple
> guarantees that the next object starts at 8-byte boundaries.)
> 

So I've rewritten this function, splitting it into four computations. I don't
know exactly where to put the align to get the correct answer.

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map.h (right):

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:14: // Provides read-only access to map data.
On 2014/10/03 18:03:38, yzshen1 wrote:
> What do you mean by it is read-only?

This was copied verbatim from array.h, and isn't true there either anymore.

https://codereview.chromium.org/611633002/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:171: class MapTypeConverterCallback {
On 2014/10/03 18:03:38, yzshen1 wrote:
> What is this for? I didn't see any usage of it.

Stray mark. I previously tried a templated Iterate() member function instead of
building my own iterator class. This was a Bad Item[tm] and this was a remnant
of that approach.

https://codereview.chromium.org/611633002/diff/100001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/611633002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:1: {%-
macro validate_field(struct, name, kind) %}
On 2014/10/03 18:03:38, yzshen1 wrote:
> Please validate that both arrays for a map are of the same size.
> (And we need to add validation tests, and maybe new validation error enum
value,
> for it. It is fine if you would like to do this part in a separate CL.)

I believe that I've now added the validation code. However, I'm not sure what I
need to do to add a validation test?

[yzshen1, 2014-10-06 07:41:53]

>> I think the current patch hasn't dealt with the serialization/etc. work
for
>> Array<Map<X, Y>>.

> Other than putting a check to make sure that we don't have an array of
maps, do
> you have any recommendations on how to deal with that?

It is too hard to explain crazy template code, so I just wrote a CL for
that: https://codereview.chromium.org/632603002/
(it is a draft that needs some more proof-reading.)

If you want, you could add a check and let me polish&land that CL.
Or you could merge it into your work. (Or please feel free to discard it if
you have a better solution.)


(And I need to continue reviewing and reply to your other comments...)

On Fri, Oct 3, 2014 at 2:11 PM, <erg@chromium.org> wrote:

> I think the current patch hasn't dealt with the serialization/etc. work for
>>
> Array<Map<X, Y>>.
>
> Other than putting a check to make sure that we don't have an array of
> maps, do
> you have any recommendations on how to deal with that?
>
>
> https://codereview.chromium.org/611633002/diff/100001/
> mojo/public/cpp/bindings/lib/map_serialization.h
> File mojo/public/cpp/bindings/lib/map_serialization.h (right):
>
> https://codereview.chromium.org/611633002/diff/100001/
> mojo/public/cpp/bindings/lib/map_serialization.h#newcode98
> mojo/public/cpp/bindings/lib/map_serialization.h:98: inline size_t
> GetSerializedSize_(const Map<MapKey, MapValue>& input) {
> On 2014/10/03 18:03:38, yzshen1 wrote:
>
>> I probably haven't understood:
>> I thought it is okay to just compute GetSerializeSize_(KeyArray) +
>> GetSerializeSize_(MapArray).
>>
>
> The problem is that there's no such thing as KeyArray or MapArray at
> this point. We have been passed this Map from a struct to get the size
> that it takes. We can't decompose the map because it's been given to us
> const. We probably don't want to Clone it because that requires the
> Value type to be Clone()able. (Also, cloning arbitrarily large data
> structures just to count the size is the Wrong Thing.)
>
> Note that this code is only called when you have a Map as a member of a
> struct.
>
>  Although the |num_bytes| field of an array is not guaranteed to be
>> 8-byte-multiple, calling GetSerializeSize_() on arrays always returns
>> 8-byte-multiple results. Therefore, I think that is sufficient. Have I
>>
> missed
>
>> anything?
>>
>
>  (The rationale is that |num_bytes| indicates the actual bytes used by
>>
> an array,
>
>> while GetSerializeSize_() is the bytes reserved, making it
>>
> 8-byte-multiple
>
>> guarantees that the next object starts at 8-byte boundaries.)
>>
>
>
> So I've rewritten this function, splitting it into four computations. I
> don't know exactly where to put the align to get the correct answer.
>
> https://codereview.chromium.org/611633002/diff/100001/
> mojo/public/cpp/bindings/map.h
> File mojo/public/cpp/bindings/map.h (right):
>
> https://codereview.chromium.org/611633002/diff/100001/
> mojo/public/cpp/bindings/map.h#newcode14
> mojo/public/cpp/bindings/map.h:14: // Provides read-only access to map
> data.
> On 2014/10/03 18:03:38, yzshen1 wrote:
>
>> What do you mean by it is read-only?
>>
>
> This was copied verbatim from array.h, and isn't true there either
> anymore.
>
> https://codereview.chromium.org/611633002/diff/100001/
> mojo/public/cpp/bindings/map.h#newcode171
> mojo/public/cpp/bindings/map.h:171: class MapTypeConverterCallback {
> On 2014/10/03 18:03:38, yzshen1 wrote:
>
>> What is this for? I didn't see any usage of it.
>>
>
> Stray mark. I previously tried a templated Iterate() member function
> instead of building my own iterator class. This was a Bad Item[tm] and
> this was a remnant of that approach.
>
> https://codereview.chromium.org/611633002/diff/100001/
> mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
> File
> mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
> (right):
>
> https://codereview.chromium.org/611633002/diff/100001/
> mojo/public/tools/bindings/generators/cpp_templates/
> struct_macros.tmpl#newcode1
> mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:1:
> {%- macro validate_field(struct, name, kind) %}
> On 2014/10/03 18:03:38, yzshen1 wrote:
>
>> Please validate that both arrays for a map are of the same size.
>> (And we need to add validation tests, and maybe new validation error
>>
> enum value,
>
>> for it. It is fine if you would like to do this part in a separate
>>
> CL.)
>
> I believe that I've now added the validation code. However, I'm not sure
> what I need to do to add a validation test?
>
> https://codereview.chromium.org/611633002/
>

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

[Elliot Glaysher, 2014-10-06 22:34:36]

So I played around with your patch on top of mine. As a first pass, it works and
compiles. Things get complicated when you try to use it in a mojom file.

For example, I threw together this struct:

  struct Arr {
    array<map<string, int>> data;
  };

And the mojom generation didn't work because GetCppType() in
mojom_cpp_generator.py doesn't know about map kinds. So, what's the parallel to
mojo::internal::Array_Data<>? I don't think it's the new MapPointerPair, since
we don't want ArrayPointer<>s, but Array_Data<> instead.

Even better, after just naively reviving my previous Map_Data<> patch on top of
this, I was able to get the compiler to crash.

[yzshen1, 2014-10-06 23:05:06]

On 2014/10/06 22:34:36, Elliot Glaysher wrote:
> So I played around with your patch on top of mine. As a first pass, it works
and
> compiles. Things get complicated when you try to use it in a mojom file.
> 
> For example, I threw together this struct:
> 
>   struct Arr {
>     array<map<string, int>> data;
>   };
> 
> And the mojom generation didn't work because GetCppType() in
> mojom_cpp_generator.py doesn't know about map kinds. So, what's the parallel
to
> mojo::internal::Array_Data<>? I don't think it's the new MapPointerPair, since
> we don't want ArrayPointer<>s, but Array_Data<> instead.


I think in this case, Arr_Data will need to have this member:
ArrayPointer<MapPointerPair<String_Data*, int>>
(MapPointerPair could be seen as two ArrayPointer<>'s, and ArrayPointer<> is a
union of uint64_t and Array_Data<>*. Therefore, MapPointerPair could be seen as
two Array_Data<>*'s. )

I think this should work because I have tried out the
serialization/deserialization in map_unittest for Array<Map<String,
Array<bool>>>, and I used the type:
  internal::Array_Data<
      internal::MapPointerPair<internal::String_Data*,
                               internal::Array_Data<bool>*>>* data;


But this kind of template code gives me a headache. I might be wrong unless I
actually try it out. :)


> 
> Even better, after just naively reviving my previous Map_Data<> patch on top
of
> this, I was able to get the compiler to crash.

I experienced some compiler crash too, when I was working on the patch and there
was some bug in my code. I think we are really pushing the limit of the
compiler. :)

[Elliot Glaysher, 2014-10-08 00:18:42]

PTAL

This switches the implementation over to a struct with two array members, as
discussed with Yuzhu. This now lets us construct arbitrarily complex data
structures, such as array<map<string, array<int32>>>. This design moves most of
the complexity out of the tmpl files and into C++ templates.

[viettrungluu, 2014-10-08 16:21:56]

https://codereview.chromium.org/611633002/diff/240001/mojo/edk/mojo_edk.gyp
File mojo/edk/mojo_edk.gyp (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/edk/mojo_edk.gyp#n...
mojo/edk/mojo_edk.gyp:89:
'../public/cpp/bindings/tests/interface_ptr_unittest.cc',
?!?

https://codereview.chromium.org/611633002/diff/240001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_data_internal.h (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:31: 
nit: extra blank line

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:24: #   generators (optional)
Possibly you should separate out the gn and gyp changes to add generators/flags
to its own CL?

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/parse/translate.py (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/parse/translate.py:21: def MapKind(kind):
Is there a reason you're making this non-private?

[Elliot Glaysher, 2014-10-08 18:20:51]

https://codereview.chromium.org/611633002/diff/240001/mojo/edk/mojo_edk.gyp
File mojo/edk/mojo_edk.gyp (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/edk/mojo_edk.gyp#n...
mojo/edk/mojo_edk.gyp:89:
'../public/cpp/bindings/tests/interface_ptr_unittest.cc',
On 2014/10/08 16:21:56, viettrungluu wrote:
> ?!?

ITT we upload before saving files. *sigh*

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:24: #   generators (optional)
On 2014/10/08 16:21:56, viettrungluu wrote:
> Possibly you should separate out the gn and gyp changes to add
generators/flags
> to its own CL?

Done: https://codereview.chromium.org/641743002/

(I'm leaving this in the current CL for now so that it passes on the trybots,
but I'll land that other CL before this one.)

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/parse/translate.py (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/parse/translate.py:21: def MapKind(kind):
On 2014/10/08 16:21:56, viettrungluu wrote:
> Is there a reason you're making this non-private?

Because pylint won't let me upload the patch if I don't. It's no longer private
and is called from unit tests. (To unit test.)

[viettrungluu, 2014-10-08 18:22:51]

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/parse/translate.py (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/parse/translate.py:21: def MapKind(kind):
On 2014/10/08 18:20:51, Elliot Glaysher wrote:
> On 2014/10/08 16:21:56, viettrungluu wrote:
> > Is there a reason you're making this non-private?
> 
> Because pylint won't let me upload the patch if I don't. It's no longer
private
> and is called from unit tests. (To unit test.)

Probably you should just suppress that pylint warning, rather than claiming that
it's part of your public interface.

[Elliot Glaysher, 2014-10-08 19:24:46]

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/parse/translate.py (right):

https://codereview.chromium.org/611633002/diff/240001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/parse/translate.py:21: def MapKind(kind):
On 2014/10/08 18:22:51, viettrungluu wrote:
> On 2014/10/08 18:20:51, Elliot Glaysher wrote:
> > On 2014/10/08 16:21:56, viettrungluu wrote:
> > > Is there a reason you're making this non-private?
> > 
> > Because pylint won't let me upload the patch if I don't. It's no longer
> private
> > and is called from unit tests. (To unit test.)
> 
> Probably you should just suppress that pylint warning, rather than claiming
that
> it's part of your public interface.

Done.

[yzshen1, 2014-10-08 21:58:33]

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:9: "interface_ptr.h",
map.h?

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_data_internal.h (right):

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:24: struct
MapDataKeyValidateParams<mojo::internal::Array_Data<char>*> {
Please add a comment saying that this is for non-nullable string.

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:33: template <typename First,
typename Second>
nit: calling them Key and Value may be more consistent with other classes.

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:49: const Map_Data*
MOJO_ALLOW_UNUSED object =
no need to have MOJO_ALLOW_UNUSED. (we have this in the generic struct case is
because we may not need to validate any fields for some structs.)

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:51: if
(!mojo::internal::ValidateEncodedPointer(&object->first.offset)) {
we are already in mojo::internal (here and elsewhere)

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:61: if
(!internal::Array_Data<First>::template Validate<
We are already in internal:: (here and elsewhere)

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:63:
mojo::internal::DecodePointerRaw(&object->first.offset),
indent 4 more spaces?

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:117: ~Map_Data() = delete;
This makes sense. And I think we should do the same for other struct data and
array data. - right?

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:119: 
maybe we can static_assert the size (of some Map_Data<> specialization) here.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_internal.h (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:121: for (size_t i = 0; i <
keys.size(); ++i)
If |keys| are not unique, then we will leak values, right?

I think it is probably hard to prevent that at the validation stage, so we
should handle it here. Does it make sense to use the Insert() method to insert
the elements?

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:174: static inline ValueConstRefType
GetValue(
optional: If we also have a non-const GetValue() version, then we can replace
most reinterpert_cast<...>(it->second.buf) with GetValue().

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:185: new ((*dst)[it->first].buf)
Value(GetValue(it).Clone().Pass());
nit:Pass() is not needed in this case.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:33: static size_t
GetHeaderSize() { return sizeof(Array_Data<DataType>); }
- Comments about the methods of MapSerializer would be helpful.
- I think GetHeaderSize() may be not necessary, because array header should
always be sizeof(ArrayHeader). Otherwise we are in bigger trouble.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:95: struct MapKeyValidateParams
{
I think you have defined these types in map_data_internal.h

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:141: // SOME SORT OF ALIGNMENT
GOES HERE?
I think you have considered all alignment properly.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:148: typename KeyWrapperType,
nit: it might be better to use consistent names for template parameters, I think
you use MapKey, MapValue, DataKey, DataValue, etc. in other methods.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validate_params.h (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:18: // ArrayValidateParams (if
elements are arrays) or NoValidateParams.
Elements may be maps, too.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map.h (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:76: // Inserts a key-value pair into the mpa.
Like std::map, this does not insert
mpa -> map

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:78: void insert(KeyForwardType key,
ValueForwardType value) {
Do we also want a remove ?

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:107: // Please note that calling this method will
fail compilation if the element
nit: element -> value

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:174: for (auto& pair : input) {
In this case, an empty std::map is converted to a NULL Map.
I think this may cause confusion.
If I convert a std::map to a Map and then send it as a non-nullable field in
Mojo messages. It will break if the std::map is empty.

(We always convert empty std::vector to empty Array.)

https://codereview.chromium.org/611633002/diff/300001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:294: def
GetMapValidateParams(kind):
Maybe use value_kind to be more clear?

https://codereview.chromium.org/611633002/diff/300001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:247: if
IsHandleKind(key_kind):
IsAnyHandleKind?

Besides, shall we also disable AnyArrayKind ?

https://codereview.chromium.org/611633002/diff/300001/mojo/services/clipboard...
File mojo/services/clipboard/clipboard_standalone_impl.cc (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.cc:33: *data =
it.GetValue().Clone().Pass();
nit: Pass() is not needed in this case. (here and in other files)

[yzshen1, 2014-10-08 22:00:27]

And about adding conformance tests:
it is fine if you would like to do it in a separate CL.
You could take a look at the Testing section in this document:
https://docs.google.com/a/chromium.org/document/d/1Zx5tWZOLvMch9SPAHMOeVzFonh...

[Elliot Glaysher, 2014-10-08 23:40:50]

Addressed Yuzhu's comments.

I plan on doing the conformance tests in the next patch.

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_data_internal.h (right):

https://codereview.chromium.org/611633002/diff/280001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:63:
mojo::internal::DecodePointerRaw(&object->first.offset),
On 2014/10/08 21:58:32, yzshen1 wrote:
> indent 4 more spaces?

This patch was run through git cl-format.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:95: struct MapKeyValidateParams
{
On 2014/10/08 21:58:33, yzshen1 wrote:
> I think you have defined these types in map_data_internal.h

The ones in map_data_internal.h are different then these with different names.
However, with some further changes to how SerializeMap_ worked, I was able to
get rid of these.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map.h (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:78: void insert(KeyForwardType key,
ValueForwardType value) {
On 2014/10/08 21:58:33, yzshen1 wrote:
> Do we also want a remove ?

I'm not sure. It was unused in this patch, and I'm fine adding it once there's a
need for it, so I'd like to punt on that.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:174: for (auto& pair : input) {
On 2014/10/08 21:58:33, yzshen1 wrote:
> In this case, an empty std::map is converted to a NULL Map.
> I think this may cause confusion.
> If I convert a std::map to a Map and then send it as a non-nullable field in
> Mojo messages. It will break if the std::map is empty.
> 
> (We always convert empty std::vector to empty Array.)

I've added an accessor to change |is_null_|. Do you have a better idea than
that, or is that fine?

[yzshen1, 2014-10-09 17:41:55]

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

[yzshen1, 2014-10-09 17:41:58]

Hi, Darin.

Would you please take a look at this comment that I made about null/empty map
construction:
https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...

I think we talked about similar things before for Array. Currently we have
Array(0) to construct an empty array, so it is not an issue. But it seems we
need to find a reasonable way for Map. And that made me revisit the
default-constructor-means-empty idea.

What do you think? Thanks!

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map.h (right):

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:78: void insert(KeyForwardType key,
ValueForwardType value) {
On 2014/10/08 23:40:49, Elliot Glaysher wrote:
> On 2014/10/08 21:58:33, yzshen1 wrote:
> > Do we also want a remove ?
> 
> I'm not sure. It was unused in this patch, and I'm fine adding it once there's
a
> need for it, so I'd like to punt on that.

That is fine. I am pretty sure people will want it, but we can add it later.

https://codereview.chromium.org/611633002/diff/300001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map.h:174: for (auto& pair : input) {
On 2014/10/08 23:40:49, Elliot Glaysher wrote:
> On 2014/10/08 21:58:33, yzshen1 wrote:
> > In this case, an empty std::map is converted to a NULL Map.
> > I think this may cause confusion.
> > If I convert a std::map to a Map and then send it as a non-nullable field in
> > Mojo messages. It will break if the std::map is empty.
> > 
> > (We always convert empty std::vector to empty Array.)
> 
> I've added an accessor to change |is_null_|. Do you have a better idea than
> that, or is that fine?

I am a little concerned that it may not be very obvious.

I am thinking about something like this:
Map<> a;  // Default to non-null.
Map<> b(nullptr);  // Set it to null.

a.clear();    // Clear to empty.
a = nullptr;  // Reset to null.

One issue with this proposal is that:
Array<Map<>> a(100);  // We are initializing 100 empty maps. This won't have a
performance difference locally, but if we send it over a message pipe, it is
significantly bigger.

I think this may be a rare case. And people can reset those maps to null if they
care, or directly push_back() null maps.

But if we choose this approach, we have to make changes to the Array
constructors (and current call sites) to be consistent. And we will need to ask
around to make sure people like this idea. I am fine if you think we should
handle this issue separately.

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:31: "lib/map.h",
map.h should be outside of lib because it is a class that third-party developers
will directly use.

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_data_internal.h (right):

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:15: // Data types for keys. The
partial specializations are different from the
The 2nd sentence is not needed anymore because those types are gone.

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_internal.h (right):

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:126: // We can't call Insert()
directly because of forwarding. Emulate the
(just out of curiosity) what is the forwarding issue? I haven't understood.

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validate_params.h (right):

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:18: // of ArrayValidateParams (if
elements are arrays, or the value type of a map
In the map case, ElementValidateParams is for the "value array", it is not for
the "elements of the value array". Therefore, whether the value type of a map is
arrays is one level deeper.

So I think this should be "if elements are arrays or maps. In the case of maps,
this is used to validate the value array" or something like that.

[Elliot Glaysher, 2014-10-09 19:15:20]

PTAL

This patch removes usage of the feature in the clipboard. I'll commit that as a
separate patch once all the generators land. (James doesn't want me to land the
part of this patch which runs only a subset of the bindings generators.)

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_internal.h (right):

https://codereview.chromium.org/611633002/diff/320001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_internal.h:126: // We can't call Insert()
directly because of forwarding. Emulate the
On 2014/10/09 17:41:58, yzshen1 wrote:
> (just out of curiosity) what is the forwarding issue? I haven't understood.

Experimented more; calling Insert() directly works if we don't call
Value.Pass(), even through it's a move only type. (Because Insert takes a
ValueRefType instead of a ValueType.)

[viettrungluu, 2014-10-09 22:22:35]

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/string.h:128: // The null string is less than a
non-NULL string.
nit: lowercase "null" please

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/string.h:130: } else if (a.is_null() || b.is_null()) {
nit: no "else"

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/string.h:131: // in all other cases where A or B is
NULL, |a| is not less than |b|.
nit: Capitalize "In" and lowercase "null".

I might write the whole thing as:

if (a.is_null())
  return !b.is_null();
if (b.is_null())
  return false;

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:436: 
nit: one more blank line (apparently), please, for consistency

[yzshen1, 2014-10-09 22:24:41]

LGTM with one last comment.

Thanks for all the great work!

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/map_unittest.cc (right):

https://codereview.chromium.org/611633002/diff/1140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/map_unittest.cc:32: class CopyableType {
Shall we move these classes which are shared with array_unittest to a separate
file?

[Elliot Glaysher, 2014-10-09 22:52:09]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-10-09 22:53:08]

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

[commit-bot: I haz the power, 2014-10-10 00:12:50]

Committed patchset #20 (id:1530001)

[commit-bot: I haz the power, 2014-10-10 00:13:40]

Patchset 20 (id:??) landed as
https://crrev.com/0f8647ec517acc9e80b0529fdb91fbd759ce85c3
Cr-Commit-Position: refs/heads/master@{#299017}