Mojo C++ bindings templates: use the same templates for user-defined structs and method params.

Mojo C++ bindings templates: use the same templates for user-defined structs and method params.

This CL removes params_defintion.tmpl and avoids several macros.

BUG=None
TEST=None
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/36e2c9b9a02d1bd81ce79c1b1ecd34ce0cb6f835

[yzshen1, 2015-02-12 23:52:40]

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

[yzshen1, 2015-02-12 23:52:40]

Please take a look.

This is the first step. I will do more cleanups.

[viettrungluu, 2015-02-13 00:51:34]

LGTM (with various comments, not to be addressed in this CL).

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:6:
{#- Validates the specified struct field, which is supposed to be an object
At least it's all in one file now, which already makes things easier.

Not to be addressed in this CL:

I wonder if it's possible to restructure this as a function (or multiple
functions) in some way (possibly with various strings and function pointers as
arguments).

I'm at least slightly concerned about the binary size impact of blind expansions
like this. (There's a space versus time tradeoff, but on the other hand bigger
code is often slower due to cache issues.)

(In the grand scheme of things, the impact of improving on things like this is
pretty small. On the other hand, generated bindings code has a well-deserved
reputation for being big and bloated.[*])

[*] And I'm not even talking about our generated code!

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:17:
"null {{name}} field in {{struct.name}} struct");
E.g., one might imagine turning this into "null %s field in %s struct" (with an
sprintf of some sort). Here, we spew the binary with lots of different strings
(none of which add any value for a user, and none of which are used in the
normal case).

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:75:
if (!data)
Personally, I find this much more readable already. ;)

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:85:
static const uint32_t kVersionSizePairs[{{struct.versions|length}}][2] = {
Readability note (not to be addressed in this CL):

* Arguably it might be more readable to declare this as

static const uint32_t kVersionSizePairs[][2] = {

Or maybe even better:

static const struct {
  uint32_t version;
  uint32_t num_bytes;
} kVersionSizes[] = {

And then use something like MOJO_ARRAYSIZE() (instead of
{{struct.versions|length}}) below.

(Also, possibly, doing kVersionSizes[MOJO_ARRAYSIZE(kVersionSizes)-1].version,
instead of {{struct.versions[-1].version}} would also be more readable for
anyone reading the generated code.)

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:92:
for (size_t i = 0; i < {{struct.versions|length}}; ++i) {
Side comment (not to be addressed in this CL):

* A linear scan is obviously suboptimal, but probably more efficient for
small-ish numbers of versions. (Don't worry about this until a .mojom with 100+
versions shows up.)
* But even for a linear scan, it's arguably better to scan starting from the
most recent versions (on the theory/hope that most apps are fairly close to the
most recent version, and penalizing old apps rather than new apps).

[yzshen1, 2015-02-17 22:07:20]

New patchsets have been uploaded after l-g-t-m from viettrungluu@chromium.org

[yzshen1, 2015-02-17 22:17:08]

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:6:
{#- Validates the specified struct field, which is supposed to be an object
On 2015/02/13 00:51:34, viettrungluu wrote:
> At least it's all in one file now, which already makes things easier.
> 
> Not to be addressed in this CL:
> 
> I wonder if it's possible to restructure this as a function (or multiple
> functions) in some way (possibly with various strings and function pointers as
> arguments).
> 
> I'm at least slightly concerned about the binary size impact of blind
expansions
> like this. (There's a space versus time tradeoff, but on the other hand bigger
> code is often slower due to cache issues.)
> 
> (In the grand scheme of things, the impact of improving on things like this is
> pretty small. On the other hand, generated bindings code has a well-deserved
> reputation for being big and bloated.[*])
> 
> [*] And I'm not even talking about our generated code!

I am thinking about converting the code into C++ template to make it a little
more readable. (Well, maybe even less readable for people that are not familiar
with C++ template...) But not sure how much that will help with reducing binary
size. (I don't know much about how modern compilers optimize templates.)

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:17:
"null {{name}} field in {{struct.name}} struct");
On 2015/02/13 00:51:34, viettrungluu wrote:
> E.g., one might imagine turning this into "null %s field in %s struct" (with
an
> sprintf of some sort). Here, we spew the binary with lots of different strings
> (none of which add any value for a user, and none of which are used in the
> normal case).

Yeah, this makes good sense. I will make such a change.

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:75:
if (!data)
On 2015/02/13 00:51:33, viettrungluu wrote:
> Personally, I find this much more readable already. ;)

Yay!

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:85:
static const uint32_t kVersionSizePairs[{{struct.versions|length}}][2] = {
On 2015/02/13 00:51:33, viettrungluu wrote:
> Readability note (not to be addressed in this CL):
> 
> * Arguably it might be more readable to declare this as
> 
> static const uint32_t kVersionSizePairs[][2] = {
> 
> Or maybe even better:
> 
> static const struct {
>   uint32_t version;
>   uint32_t num_bytes;
> } kVersionSizes[] = {
> 
> And then use something like MOJO_ARRAYSIZE() (instead of
> {{struct.versions|length}}) below.
> 
> (Also, possibly, doing kVersionSizes[MOJO_ARRAYSIZE(kVersionSizes)-1].version,
> instead of {{struct.versions[-1].version}} would also be more readable for
> anyone reading the generated code.)

That makes sense.
I will make it in a separate CL, because I would like to only move things around
in this CL.

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:92:
for (size_t i = 0; i < {{struct.versions|length}}; ++i) {
On 2015/02/13 00:51:34, viettrungluu wrote:
> Side comment (not to be addressed in this CL):
> 
> * A linear scan is obviously suboptimal, but probably more efficient for
> small-ish numbers of versions. (Don't worry about this until a .mojom with
100+
> versions shows up.)
> * But even for a linear scan, it's arguably better to scan starting from the
> most recent versions (on the theory/hope that most apps are fairly close to
the
> most recent version, and penalizing old apps rather than new apps).

Yeah. That makes sense. Thanks! I will make the change in a follow-up CL.

[yzshen1, 2015-02-17 22:18:18]

Committed patchset #3 (id:40001) manually as
36e2c9b9a02d1bd81ce79c1b1ecd34ce0cb6f835 (presubmit successful).

[viettrungluu, 2015-02-17 22:32:50]

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:6:
{#- Validates the specified struct field, which is supposed to be an object
On 2015/02/17 22:17:08, yzshen1 wrote:
> On 2015/02/13 00:51:34, viettrungluu wrote:
> > At least it's all in one file now, which already makes things easier.
> > 
> > Not to be addressed in this CL:
> > 
> > I wonder if it's possible to restructure this as a function (or multiple
> > functions) in some way (possibly with various strings and function pointers
as
> > arguments).
> > 
> > I'm at least slightly concerned about the binary size impact of blind
> expansions
> > like this. (There's a space versus time tradeoff, but on the other hand
bigger
> > code is often slower due to cache issues.)
> > 
> > (In the grand scheme of things, the impact of improving on things like this
is
> > pretty small. On the other hand, generated bindings code has a well-deserved
> > reputation for being big and bloated.[*])
> > 
> > [*] And I'm not even talking about our generated code!
> 
> I am thinking about converting the code into C++ template to make it a little
> more readable. (Well, maybe even less readable for people that are not
familiar
> with C++ template...) But not sure how much that will help with reducing
binary
> size. (I don't know much about how modern compilers optimize templates.)

A C++ template is one definite possibility, though templates can come with
binary size issues (as well as compilation time issues, if they're in headers).

Another possibility is to use classical polymorphism, etc. (This might sometimes
win on binary size, with the cost being some indirection/vtable thunks.)

You might want to use some combination of the two (in addition to pure generated
code), as appropriate. (Both have the advantage that you can read the code and
test it, without involving the bindings generator.)

[yzshen1, 2015-02-17 22:42:31]

On 2015/02/17 22:32:50, viettrungluu wrote:
>
https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
> File
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
> (right):
> 
>
https://codereview.chromium.org/917333003/diff/20001/mojo/public/tools/bindin...
> mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:6:
> {#- Validates the specified struct field, which is supposed to be an object
> On 2015/02/17 22:17:08, yzshen1 wrote:
> > On 2015/02/13 00:51:34, viettrungluu wrote:
> > > At least it's all in one file now, which already makes things easier.
> > > 
> > > Not to be addressed in this CL:
> > > 
> > > I wonder if it's possible to restructure this as a function (or multiple
> > > functions) in some way (possibly with various strings and function
pointers
> as
> > > arguments).
> > > 
> > > I'm at least slightly concerned about the binary size impact of blind
> > expansions
> > > like this. (There's a space versus time tradeoff, but on the other hand
> bigger
> > > code is often slower due to cache issues.)
> > > 
> > > (In the grand scheme of things, the impact of improving on things like
this
> is
> > > pretty small. On the other hand, generated bindings code has a
well-deserved
> > > reputation for being big and bloated.[*])
> > > 
> > > [*] And I'm not even talking about our generated code!
> > 
> > I am thinking about converting the code into C++ template to make it a
little
> > more readable. (Well, maybe even less readable for people that are not
> familiar
> > with C++ template...) But not sure how much that will help with reducing
> binary
> > size. (I don't know much about how modern compilers optimize templates.)
> 
> A C++ template is one definite possibility, though templates can come with
> binary size issues (as well as compilation time issues, if they're in
headers).
> 
> Another possibility is to use classical polymorphism, etc. (This might
sometimes
> win on binary size, with the cost being some indirection/vtable thunks.)
> 
> You might want to use some combination of the two (in addition to pure
generated
> code), as appropriate. (Both have the advantage that you can read the code and
> test it, without involving the bindings generator.)

Thanks for the suggestion. I will think more about it and hopefully come up with
a CL. :)