Cleanup: Some simplifications in json_schema_compiler.

Cleanup: Some simplifications in json_schema_compiler.

There were several GetItemFromList functions that extract an element from a ListValue and converted it.
This change consolidates the list access into a single place and simplifies the GetItemFromList to the conversion only (renamed to PopulateItem).

No functional change.

Committed: https://crrev.com/de496d5995a59c568f594f49fb2f76cefb4df54c
Cr-Commit-Position: refs/heads/master@{#312082}

[pneubeck (no reviews), 2015-01-13 15:09:37]

Patchset #1 (id:1) has been deleted

[pneubeck (no reviews), 2015-01-13 15:10:46]

pneubeck@chromium.org changed reviewers:
+ kalman@chromium.org

[pneubeck (no reviews), 2015-01-13 15:10:47]

if you're ok with this solution then I can add more documentation of the
binary_tag.

this was the least invasive change that i could think of.
alternatives will probably require more code duplication or a larger change.

[not at google - send to devlin, 2015-01-13 18:15:13]

kalman@chromium.org changed reviewers:
+ jyasskin@chromium.org

[not at google - send to devlin, 2015-01-13 18:15:14]

Probably TMI here. Sorry about the nits.

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util.h:20: struct ITEMS_ARE_NOT_BINARY {};
Mhm ok I'm going to add Jeffrey here to advise on the most idiomatic way of
doing this. Perhaps in the new world there's an easy way to do all this, e.g.
without the structs and needing to instantiate them etc. Maybe a boolean? Maybe
C++11 provides an easy way?

For example, I did a bunch of playing, and it looks like you can avoid the icky
marker struct (and cut down a tiny bit on binary size by generating less code;
or at least, make it more readable) with something like:

template <typename OutType>
using GetItemFromListStrategy =
    bool(const base::ListValue&, int, OutType*);

// implementations of each strategy
bool GetIntFromList(
    const base::ListValue& list, int index, int* out);
bool GetDoubleFromList(
    const base::ListValue& list, int index, double* out);
...
bool GetBinaryFromList(
    const base::ListValue& list, int index, base::BinaryValue* out);

then

template <typename OutType>
bool PopulateArrayFromList(
    const base::ListValue& from,
    GetItemFromListStrategy<OutType> strategy,
    std::vector<OutType>* out) {
  ...  // that implementation
}

and then you'd generate slightly different code (more, but less magical), which
needs to pass in the strategy, but it avoids some templates?

But I'm not sure that's any better. I defer to Jeffrey.

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/util_cc_helper.py (right):

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util_cc_helper.py:39: def PopulateArrayFromList(self,
src, dst, optional, items_are_binary):
For boolean flags I prefer to make it explicit, like binary=False then forcing
callers to write binary=<expression>

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util_cc_helper.py:54: '(*%(src)s,
%(namespace)s::%(binary_tag)s(), &%(dst)s)')
Given there's even more duplication, you should be able to sugarise it a little
more, like

optional_or_required = 'Optional' if optional else ''
binary_tag = 'ITEMS_ARE_BINARY' if binary else 'ITEMS_ARE_NOT_BINARY'
return '%s::Populate%sFromList(*%s), %s::%s(), &%s)' % ...

(I don't particularly like named string substitution params in these cases, just
obfuscates the code IMO)

[Jeffrey Yasskin, 2015-01-13 18:57:09]

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util.h:20: struct ITEMS_ARE_NOT_BINARY {};
On 2015/01/13 18:15:13, kalman wrote:
> Mhm ok I'm going to add Jeffrey here to advise on the most idiomatic way of
> doing this. Perhaps in the new world there's an easy way to do all this, e.g.
> without the structs and needing to instantiate them etc. Maybe a boolean?
Maybe
> C++11 provides an easy way?
> 
> For example, I did a bunch of playing, and it looks like you can avoid the
icky
> marker struct (and cut down a tiny bit on binary size by generating less code;
> or at least, make it more readable) with something like:
> 
> template <typename OutType>
> using GetItemFromListStrategy =
>     bool(const base::ListValue&, int, OutType*);
> 
> // implementations of each strategy
> bool GetIntFromList(
>     const base::ListValue& list, int index, int* out);
> bool GetDoubleFromList(
>     const base::ListValue& list, int index, double* out);
> ...
> bool GetBinaryFromList(
>     const base::ListValue& list, int index, base::BinaryValue* out);
> 
> then
> 
> template <typename OutType>
> bool PopulateArrayFromList(
>     const base::ListValue& from,
>     GetItemFromListStrategy<OutType> strategy,
>     std::vector<OutType>* out) {
>   ...  // that implementation
> }
> 
> and then you'd generate slightly different code (more, but less magical),
which
> needs to pass in the strategy, but it avoids some templates?
> 
> But I'm not sure that's any better. I defer to Jeffrey.

Function pointers are generally slow to call, although we don't usually worry
much about performance in the extensions bindings, right?

The core problem here is that we want to store both UTF-8 strings and
ArrayBuffers in std::string? Would it make sense to store the ArrayBuffers into
a vector<unsigned char>, and then use the existing overloading to deal with it?

[not at google - send to devlin, 2015-01-13 19:39:56]

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util.h:20: struct ITEMS_ARE_NOT_BINARY {};
> Function pointers are generally slow to call, although we don't usually worry
> much about performance in the extensions bindings, right?

I'm fairly certain a lot more is going on than calling function pointers.
Another tradeoff is this vs binary size, but either way it's a miniscule effect
and shouldn't affect things. Code cleanliness is what I like optimising for.

> 
> The core problem here is that we want to store both UTF-8 strings and
> ArrayBuffers in std::string? Would it make sense to store the ArrayBuffers
into
> a vector<unsigned char>, and then use the existing overloading to deal with
it?

Ah right, yeah, that's a good idea. It would be a really simple change on the
schema compiler's side - I'm not sure how much of a change on the caller's side.

Hm, along that vein, could you do something like:

typedef std::string BinaryOut;
or
using std::string = BinaryOut;

to make the template system behave better?

[Jeffrey Yasskin, 2015-01-13 21:17:21]

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util.h:20: struct ITEMS_ARE_NOT_BINARY {};
On 2015/01/13 19:39:56, kalman wrote:
> > The core problem here is that we want to store both UTF-8 strings and
> > ArrayBuffers in std::string? Would it make sense to store the ArrayBuffers
> into
> > a vector<unsigned char>, and then use the existing overloading to deal with
> it?
> 
> Ah right, yeah, that's a good idea. It would be a really simple change on the
> schema compiler's side - I'm not sure how much of a change on the caller's
side.
> 
> Hm, along that vein, could you do something like:
> 
> typedef std::string BinaryOut;
> or
> using std::string = BinaryOut;
> 
> to make the template system behave better?

Nope. :)

[pneubeck (no reviews), 2015-01-14 10:48:23]

Patchset #2 (id:40001) has been deleted

[pneubeck (no reviews), 2015-01-14 10:54:15]

Changed to approximately what Benjamin suggested however making use of C++
overloading to automatically select the right 'strategy' (I called it
'PopulateItem').

Only for Binary an other populateItem function is used.

To simplify all of this, I refactored the redundant GetItemFromList functions
(renamed to PopulateItem) and consolidated the various calls to
ListValue::Get*() into a single place (namely in PopulateArrayFromList).

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/util_cc_helper.py (right):

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util_cc_helper.py:39: def PopulateArrayFromList(self,
src, dst, optional, items_are_binary):
On 2015/01/13 18:15:14, kalman wrote:
> For boolean flags I prefer to make it explicit, like binary=False then forcing
> callers to write binary=<expression>
Not sure how you could /force/ the caller to use named arguments, as he can
always just pass in positional arguments and python will be happy, AFAIK.

Done.

https://codereview.chromium.org/851673003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/util_cc_helper.py:54: '(*%(src)s,
%(namespace)s::%(binary_tag)s(), &%(dst)s)')
On 2015/01/13 18:15:14, kalman wrote:
> Given there's even more duplication, you should be able to sugarise it a
little
> more, like
> 
> optional_or_required = 'Optional' if optional else ''
> binary_tag = 'ITEMS_ARE_BINARY' if binary else 'ITEMS_ARE_NOT_BINARY'
> return '%s::Populate%sFromList(*%s), %s::%s(), &%s)' % ...
> 
> (I don't particularly like named string substitution params in these cases,
just
> obfuscates the code IMO)

Slight variation of what you proposed because concatenating identifiers is IMO a
no-go as it destroys grep-ability.

[pneubeck (no reviews), 2015-01-14 10:55:22]

pneubeck@chromium.org changed reviewers:
+ mark@chromium.org

[pneubeck (no reviews), 2015-01-14 10:55:23]

mark@chromium.org: Please review changes in base/values*

[Mark Mentovai, 2015-01-14 14:39:59]

LGTM

https://codereview.chromium.org/851673003/diff/80001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/851673003/diff/80001/base/values.h#newcode82
base/values.h:82: virtual bool GetAsBinary(std::string* out_value) const;
Can you keep these methods sorted like the Type enum above?

[not at google - send to devlin, 2015-01-14 21:22:47]

Philip - sorry to run you around here, but I do think that Jeffrey's suggestion
was better. I had a crack at what might happen if you try doing it:

https://codereview.chromium.org/850173002/

and as expected it causes a number of compile failures in the hardware API
implementations - but not too many. What's more, it looks like fixing those
compile errors would be an overall improvement in the code, because these byte
arrays don't really want to be std::strings.

Was there another reason why it's not feasible to change the type like that? If
so, happy to go ahead with this patch.

[pneubeck (no reviews), 2015-01-15 09:53:36]

Patchset #4 (id:100001) has been deleted

[pneubeck (no reviews), 2015-01-15 10:01:40]

pneubeck@chromium.org changed reviewers:
- mark@chromium.org

[pneubeck (no reviews), 2015-01-15 10:01:40]

-Mark

your discussion with Jeffrey didn't come to a conclusion and, independent of our
issues with the json_schema_compiler, I don't see a clear reason why vector<>
should be preferred over string for binary data. The argument that string should
be reserved for utf8 and using the string for both binary and utf8 might lead to
confusion, seems to be a rather weak one. This is also indicated by the many
places that use string for binary and many that use vector in the repository.
E.g. the code that I call in the api uses string:
net/ssl/ssl_cert_request_info.h:53 (std::vector<std::string> cert_authorities).

Anyways, I don't think that there are strong arguments in either direction, so
let's go with vector<>.


I changed this CL to keep at least the other collateral changes.

[not at google - send to devlin, 2015-01-15 20:44:11]

On 2015/01/15 10:01:40, pneubeck wrote:
> -Mark
> 
> your discussion with Jeffrey didn't come to a conclusion

Yeah, sorry. My "that's a good idea" was an implicit deference to Jeffrey,
apologies that wasn't clear.

> and, independent of our
> issues with the json_schema_compiler, I don't see a clear reason why vector<>
> should be preferred over string for binary data.

From what I've noticed in Chromium code, binary is more typically represented as
a vector<uint8> or vector<char> (unfortunately, both) rather than std::string.
Certainly, when I changed the type to be vector<char> then mostly I found
callers converting to/from their own representation - typically a
vector<char|utf8> - from std::string.

> The argument that string should
> be reserved for utf8 and using the string for both binary and utf8 might lead
to
> confusion, seems to be a rather weak one. This is also indicated by the many
> places that use string for binary and many that use vector in the repository.
> E.g. the code that I call in the api uses string:
> net/ssl/ssl_cert_request_info.h:53 (std::vector<std::string>
cert_authorities).

Ah right. Yes this is a pain, I'm not familiar with that, just the hardware API
callers.

> 
> Anyways, I don't think that there are strong arguments in either direction, so
> let's go with vector<>.

I do think that changing this on the type level is a little simpler to
understand - every JSON type maps onto a C++ type, that's a nice invariant.

> 
> 
> I changed this CL to keep at least the other collateral changes.

[not at google - send to devlin, 2015-01-15 20:47:30]

lgtm

https://codereview.chromium.org/851673003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/851673003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/util.h:49: for (size_t i = 0; i < list.GetSize();
++i) {
I notice that ListValue has an iterator-like interface with begin() and end(), I
wonder if the (const base::Value* value : list) syntax works here? That'd be
simpler. There's no way that "list.Get(i, &value)" should ever be able to fail.

[pneubeck (no reviews), 2015-01-16 08:20:14]

https://codereview.chromium.org/851673003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/851673003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/util.h:49: for (size_t i = 0; i < list.GetSize();
++i) {
On 2015/01/15 20:47:30, kalman wrote:
> I notice that ListValue has an iterator-like interface with begin() and end(),
I
> wonder if the (const base::Value* value : list) syntax works here? That'd be
> simpler. There's no way that "list.Get(i, &value)" should ever be able to
fail.

Yes, forgot about that.
Did the same to the other for loop below.

[pneubeck (no reviews), 2015-01-16 08:20:17]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-01-16 08:20:21]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-01-16 08:20:27]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-01-16 08:21:12]

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

[commit-bot: I haz the power, 2015-01-16 08:23:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-16 08:23:54]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[pneubeck (no reviews), 2015-01-16 16:25:00]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-01-16 16:25:49]

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

[commit-bot: I haz the power, 2015-01-16 16:26:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-16 16:26:23]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pneubeck (no reviews), 2015-01-16 16:32:14]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-01-16 16:32:43]

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

[commit-bot: I haz the power, 2015-01-16 17:57:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-16 17:57:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pneubeck (no reviews), 2015-01-16 21:07:24]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-01-16 21:08:16]

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

[commit-bot: I haz the power, 2015-01-16 21:09:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-16 21:09:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pneubeck (no reviews), 2015-01-19 08:48:53]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-01-19 08:49:19]

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

[commit-bot: I haz the power, 2015-01-19 08:50:21]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-01-19 08:51:13]

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