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

mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl

-{%- import "struct_macros.tmpl" as struct_macros %}
 {%- set class_name = struct.name ~ "_Data" %}
 
+{#- TODO(yzshen): Consider eliminating _validate_object() and
+    _validate_handle(). #}
+
+{#- Validates the specified struct field, which is supposed to be an object

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

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!

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

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.)

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

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.)

+    (struct/array/string/map/union).
+    This macro is expanded by the Validate() method. #}
+{%- macro _validate_object(struct, packed_field) %}
+{%-   set name = packed_field.field.name %}
+{%-   set kind = packed_field.field.kind %}
+{%-   set wrapper_type = kind|cpp_wrapper_type %}
+{%-   if not kind|is_nullable_kind %}
+  if (!object->{{name}}.offset) {
+    ReportValidationError(
+        mojo::internal::VALIDATION_ERROR_UNEXPECTED_NULL_POINTER,
+        "null {{name}} field in {{struct.name}} struct");

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

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).

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

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

+    return false;
+  }
+{%-   endif %}
+  if (!mojo::internal::ValidateEncodedPointer(&object->{{name}}.offset)) {
+    ReportValidationError(mojo::internal::VALIDATION_ERROR_ILLEGAL_POINTER);
+    return false;
+  }
+{%-   if kind|is_array_kind or kind|is_string_kind %}
+  if (!{{wrapper_type}}::Data_::Validate<
+          {{kind|get_array_validate_params|indent(10)}}>(
+              mojo::internal::DecodePointerRaw(&object->{{name}}.offset),
+              bounds_checker)) {
+{%-   elif kind|is_map_kind %}
+  if (!{{wrapper_type}}::Data_::Validate<
+          {{kind.value_kind|get_map_validate_params|indent(10)}}>(
+              mojo::internal::DecodePointerRaw(&object->{{name}}.offset),
+              bounds_checker)) {
+{%-   elif kind|is_struct_kind %}
+  if (!{{kind|get_name_for_kind}}::Data_::Validate(
+          mojo::internal::DecodePointerRaw(&object->{{name}}.offset),
+          bounds_checker)) {
+{%-   else %}
+  if (!{{wrapper_type}}::Data_::Validate(
+          mojo::internal::DecodePointerRaw(&object->{{name}}.offset),
+          bounds_checker)) {
+{%-   endif %}
+    return false;
+  }
+{%- endmacro %}
+
+{#- Validates the specified struct field, which is supposed to be a handle.
+    This macro is expanded by the Validate() method. #}
+{%- macro _validate_handle(struct, packed_field) %}
+{%-   set name = packed_field.field.name %}
+{%-   set kind = packed_field.field.kind %}
+{%-   if not kind|is_nullable_kind %}
+  if (object->{{name}}.value() == mojo::internal::kEncodedInvalidHandleValue) {
+    ReportValidationError(
+        mojo::internal::VALIDATION_ERROR_UNEXPECTED_INVALID_HANDLE,
+        "invalid {{name}} field in {{struct.name}} struct");
+    return false;
+  }
+{%-   endif %}
+  if (!bounds_checker->ClaimHandle(object->{{name}})) {
+    ReportValidationError(mojo::internal::VALIDATION_ERROR_ILLEGAL_HANDLE);
+    return false;
+  }
+{%- endmacro %}
+
 // static
 {{class_name}}* {{class_name}}::New(mojo::internal::Buffer* buf) {
   return new (buf->Allocate(sizeof({{class_name}}))) {{class_name}}();
 }
 
 // static
 bool {{class_name}}::Validate(const void* data,
                               mojo::internal::BoundsChecker* bounds_checker) {
-  {{struct_macros.validate(struct)}}
+  if (!data)

[viettrungluu, 2015/02/13 00:51:33]

Personally, I find this much more readable already. ;)

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

Yay!

+    return true;
+
+  if (!ValidateStructHeaderAndClaimMemory(data, bounds_checker))
+    return false;
+
+  // NOTE: The memory backing |object| may be smaller than |sizeof(*object)| if
+  // the message comes from an older version.
+  const {{class_name}}* object = static_cast<const {{class_name}}*>(data);
+
+  static const uint32_t kVersionSizePairs[{{struct.versions|length}}][2] = {

[viettrungluu, 2015/02/13 00:51:33]

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.)

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

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

+{%- for version in struct.versions -%}
+    { {{version.version}}, {{version.num_bytes}} }{% if not loop.last %}, {% endif -%}
+{%- endfor -%}
+  };
+
+  if (object->header_.version <= {{struct.versions[-1].version}}) {
+    for (size_t i = 0; i < {{struct.versions|length}}; ++i) {

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

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:17:08]

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

+      if (object->header_.version <= kVersionSizePairs[i][0]) {
+        if (object->header_.num_bytes == kVersionSizePairs[i][1])
+          break;
+
+        ReportValidationError(
+            mojo::internal::VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
+        return false;
+      }
+    }
+  } else if (object->header_.num_bytes < {{struct.versions[-1].num_bytes}}) {
+    ReportValidationError(
+        mojo::internal::VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
+    return false;
+  }
+
+{#- Before validating fields introduced at a certain version, we need to add
+    a version check, which makes sure we skip further validation if |object|
+    is from an earlier version. |last_checked_version| records the last
+    version that we have added such version check. #}
+{%- set last_checked_version = 0 %}
+{%- for packed_field in struct.packed.packed_fields_in_ordinal_order %}
+{%-   set kind = packed_field.field.kind %}
+{%-   if kind|is_object_kind or kind|is_any_handle_kind %}
+{%-     if packed_field.min_version > last_checked_version %}
+{%-       set last_checked_version = packed_field.min_version %}
+  if (object->header_.version < {{packed_field.min_version}})
+    return true;
+{%-     endif %}
+{%-     if kind|is_object_kind %}
+{{_validate_object(struct, packed_field)}}
+{%-     elif kind|is_any_handle_kind %}
+{{_validate_handle(struct, packed_field)}}
+{%-     endif %}
+{%-   endif %}
+{%- endfor %}
+
+  return true;
+}
+
+void {{class_name}}::EncodePointersAndHandles(
+    std::vector<mojo::Handle>* handles) {
+  MOJO_CHECK(header_.version == {{struct.versions[-1].version}});
+{%- for pf in struct.packed.packed_fields_in_ordinal_order %}
+{%-   if pf.field.kind|is_object_kind %}
+  mojo::internal::Encode(&{{pf.field.name}}, handles);
+{%-   elif pf.field.kind|is_any_handle_kind %}
+  mojo::internal::EncodeHandle(&{{pf.field.name}}, handles);
+{%-   endif %}
+{%- endfor %}
+}
+
+void {{class_name}}::DecodePointersAndHandles(
+    std::vector<mojo::Handle>* handles) {
+  // NOTE: The memory backing |this| may has be smaller than |sizeof(*this)|, if
+  // the message comes from an older version.
+{#- Before decoding fields introduced at a certain version, we need to add
+    a version check, which makes sure we skip further decoding if |this|
+    is from an earlier version. |last_checked_version| records the last
+    version that we have added such version check. #}
+{%- set last_checked_version = 0 %}
+{%- for pf in struct.packed.packed_fields_in_ordinal_order %}
+{%-   set name = pf.field.name %}
+{%-   set kind = pf.field.kind %}
+{%-   if kind|is_object_kind or kind|is_any_handle_kind %}
+{%-     if pf.min_version > last_checked_version %}
+{%-       set last_checked_version = pf.min_version %}
+  if (header_.version < {{pf.min_version}})
+    return;
+{%-     endif %}
+{%-     if kind|is_object_kind %}
+  mojo::internal::Decode(&{{name}}, handles);
+{%-     else %}
+  mojo::internal::DecodeHandle(&{{name}}, handles);
+{%-     endif %}
+{%-   endif %}
+{%- endfor %}
 }
 
 {{class_name}}::{{class_name}}() {
   header_.num_bytes = sizeof(*this);
   header_.version = {{struct.versions[-1].version}};
 }
-
-void {{class_name}}::EncodePointersAndHandles(
-    std::vector<mojo::Handle>* handles) {
-  {{struct_macros.encodes(struct)}}
-}
-
-void {{class_name}}::DecodePointersAndHandles(
-    std::vector<mojo::Handle>* handles) {
-  {{struct_macros.decodes(struct)}}
-}