Add output formatters for more mojom types.

Add output formatters for more mojom types.

Add output formatters for printing the contents of Mojo arrays,
maps, and structs.  These are very useful for writing debug logs.

Review URL: https://codereview.chromium.org/1519673002 .

R=viettrungluu@chromium.org, abarth, jamesr, viettrungluu
BUG=

Committed: https://chromium.googlesource.com/external/mojo/+/e60e570696af32536f1a8e4bc7f8ce8935d6552d

[viettrungluu, 2015-12-17 23:26:29]

To quote and reply to what you said in the original review:

> Some good points however I feel we can be a little more opinionated about the
> behavior of our framework.
>
> 1. Due to ADL, clients of Mojo would have difficulty defining operator
overloads
> for Mojo types in their own namespace anyhow.  If their formatting needs are
> especially particular then I can easily envision them wanting to define their
> own stream type rather than use std::ostream and being beholden to the
standard
> library's own behavior.

There's no restriction in them defining stuff in the mojo namespace. (We could a
"soft" restriction against it, though it'd carry even less weight than when,
e.g., the C++ standard does it.)

(It'd be more logical to use our own stream type for debugging purposes, but
that ship has already sailed.)

> 2. I can remove this phrase.  Really I just wanted something human readable
> which is the general intent of the standard library output formatters anyhow. 
> These formatters are consistent with that aim.

Well, the standard library's formatters tend to have various bells and whistles
for formatting, and aren't just for debugging.

> 3. I tried this approach and it created some challenges.  a) There isn't a
clear
> place for the declarations to go.  b) Nearly everyone who defines output
> formatters ends up wanting to include it.  c) Output formatters are easier to
> find next to their related types for API usability.

(To be clear, I was just talking about formatters for things in
mojo/public/cpp/bindings.)

> 4. I think we can cross that bridge when we get to it.  For now the Array,
Map,
> and String types belong to Mojo so it makes sense for us to specify how they
are
> formatted.  If we change the representation then there will be lots of other
> things to fix at the same time and we can revisit our options then.

I'd really like to do this sooner rather than later, and adding these would only
make that task even harder than it already is.

> 5. Mojo::ToDebugString is pretty clunky.  Moreover I really don't want to be
> generating intermediate strings all over the place.  Output formatters already
> neatly solve this problem and are compatible with our logging infrastructure.

For logging purposes, I don't think the cost of intermediate strings is a
significant concern.

> All things considered I would very much like to proceed with the current
> approach and see whether it causes any trouble down the road.

My preference is for a function like mojo::ToDebugString() (or even
mojo::ToString()):
* In the case of "Debug", it's nicely explicit about its purpose.
* It gives an easy way for you to specify options. E.g., often you may want to
limit the (maximum) number of items output -- for debug purposes, you probably
often don't want huge arrays/maps output. You could then have something like:

// max_items = 0 means all items.
template <typename T>
std::string mojo::ToDebugString(const mojo::Array<T>& a, size_t max_items = 0);

> The big argument for me is API usability.  I don't think we lose anything by
> defining these functions.

I prefer to start with being explicit, if maybe a bit inconvenient, and go from
there.

(I should also point out that I haven't heard a huge demand for logging these
types, including from Mandoline/Chromium.)

[jeffbrown, 2016-01-16 01:01:30]

I need feedback on this to unblock Mozart.

[jeffbrown, 2016-01-26 09:04:29]

rebase

[viettrungluu, 2016-01-26 18:44:05]

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/formatting.h (right):

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/formatting.h:40: template <typename Key, typename
Value>
This is maybe a bit bikesheddy, but I think that the world (JS/JSON, Python,
Dart, Go, ...) has mostly agreed to format maps as {key1: value1, key2: value2}
(with varying spacing/indentation).

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/array_unittest.cc (right):

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/array_unittest.cc:832: TEST_F(ArrayTest,
OutputFormatting) {
Could you move all these tests to a formatting_unittest.cc (and presumably tests
called FormattingTest.{Array, Map, ...})?

[jeffbrown, 2016-01-26 22:57:31]

move tests, change map formatting

[jeffbrown, 2016-01-26 22:57:59]

PTAL.

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/formatting.h (right):

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/formatting.h:40: template <typename Key, typename
Value>
On 2016/01/26 18:44:05, viettrungluu wrote:
> This is maybe a bit bikesheddy, but I think that the world (JS/JSON, Python,
> Dart, Go, ...) has mostly agreed to format maps as {key1: value1, key2:
value2}
> (with varying spacing/indentation).

Done.

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/array_unittest.cc (right):

https://codereview.chromium.org/1532993002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/array_unittest.cc:832: TEST_F(ArrayTest,
OutputFormatting) {
On 2016/01/26 18:44:05, viettrungluu wrote:
> Could you move all these tests to a formatting_unittest.cc (and presumably
tests
> called FormattingTest.{Array, Map, ...})?

Done.

[viettrungluu, 2016-01-26 23:19:40]

lgtm w/nit

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/formatting_unittest.cc (right):

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/formatting_unittest.cc:8: #include
"mojo/public/cpp/bindings/formatting.h"
nit: The convention is to include the particular file, in this case
formatting.h, first in the unit test. (Not a big deal -- it helps to check that
the .h file is independently compilable when there's no .cc file.)

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/formatting_unittest.cc:15: namespace {
Note: You can probably put everything in an anonymous namespace (the only thing
which you may not be able to put in there are the operator<<s), including the
TEST() { ... } parts.

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/formatting_unittest.cc:78: "one_element_map={123:
abc}, "
Side comment (not for this CL -- for future reference):

It'd be nice if abc were quoted here, which indicates the desirability of some
distinction along the lines of Python's str() vs repr() ("%s" vs "%r")....

I wonder if the right thing to do isn't to have formatting functions (whether
they deal with strings or streams; I guess streams are more general)
included/available everywhere, and then have formatting.h use those functions
(picking either repr or str).

[jeffbrown, 2016-01-26 23:40:46]

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/formatting_unittest.cc (right):

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/formatting_unittest.cc:8: #include
"mojo/public/cpp/bindings/formatting.h"
On 2016/01/26 23:19:40, viettrungluu wrote:
> nit: The convention is to include the particular file, in this case
> formatting.h, first in the unit test. (Not a big deal -- it helps to check
that
> the .h file is independently compilable when there's no .cc file.)

Yeah, none of the other unit tests in this directory do that so I followed the
local convention.  Ordinarily I do as you suggest.

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/formatting_unittest.cc:15: namespace {
On 2016/01/26 23:19:40, viettrungluu wrote:
> Note: You can probably put everything in an anonymous namespace (the only
thing
> which you may not be able to put in there are the operator<<s), including the
> TEST() { ... } parts.

Acknowledged.

https://codereview.chromium.org/1532993002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/formatting_unittest.cc:78: "one_element_map={123:
abc}, "
On 2016/01/26 23:19:40, viettrungluu wrote:
> Side comment (not for this CL -- for future reference):
> 
> It'd be nice if abc were quoted here, which indicates the desirability of some
> distinction along the lines of Python's str() vs repr() ("%s" vs "%r")....
> 
> I wonder if the right thing to do isn't to have formatting functions (whether
> they deal with strings or streams; I guess streams are more general)
> included/available everywhere, and then have formatting.h use those functions
> (picking either repr or str).

Starting to think you may be right, but the lack of quotes should make it clear
why I did the array-style thing before since it tended to be less ambiguous.  :)

Some kind of overloaded mojo::Format function might do nicely.  Stuff to think
about later.

[jeffbrown, 2016-01-26 23:50:29]

Description was changed from

==========
Add output formatters for more mojom types.

Add output formatters for printing the contents of Mojo arrays,
maps, and structs.  These are very useful for writing debug logs.

Review URL: https://codereview.chromium.org/1519673002 .

R=abarth, jamesr, viettrungluu
BUG=
==========

to

==========
Add output formatters for more mojom types.

Add output formatters for printing the contents of Mojo arrays,
maps, and structs.  These are very useful for writing debug logs.

Review URL: https://codereview.chromium.org/1519673002 .

R=viettrungluu@chromium.org, abarth, jamesr, viettrungluu
BUG=

Committed:
https://chromium.googlesource.com/external/mojo/+/e60e570696af32536f1a8e4bc7f...
==========

[jeffbrown, 2016-01-26 23:50:30]

Committed patchset #5 (id:80001) manually as
e60e570696af32536f1a8e4bc7f8ce8935d6552d (presubmit successful).