Perform field initialization to defaults only when the field is skipped.

Avoids unnecessary initialization & unnecessary scan of
the pb_field_t array.

Runtime on Cortex-M3 -5%, code size +2%.

Could need some more testing with random field orders.
Have to write a tool to randomize fields in a message.

CHANGELOG

@@ -1,20 +1,3 @@
-nanopb-0.2.1
- NOTE: The default callback function signature has changed.
-       If you don't want to update your code, define PB_OLD_CALLBACK_STYLE.
- 
- Change the callback function to use void** (issue 69)
- Add support for defining the nanopb options in a separate file (issue 12)
- Add support for packed structs in IAR and MSVC (in addition to GCC) (issue 66)
- Implement error message support for the encoder side (issue 7)
- Handle unterminated strings when encoding (issue 68)
- Fix bug with empty strings in repeated string callbacks (issue 73)
- Fix regression in 0.2.0 with optional callback fields (issue 70)
- Fix bugs with empty message types (issues 64, 65)
- Fix some compiler warnings on clang (issue 67)
- Some portability improvements (issues 60, 62)
- Various new generator options
- Improved tests
-
 nanopb-0.2.0
  NOTE: This release requires you to regenerate all .pb.c
        files. Files generated by older versions will not

generator/nanopb_generator.py

@@ -1,5 +1,5 @@
 '''Generate header file for nanopb from a ProtoBuf FileDescriptorSet.'''
-nanopb_version = "nanopb-0.2.1"
+nanopb_version = "nanopb-0.2.1-dev"
 
 try:
     import google.protobuf.descriptor_pb2 as descriptor

pb.h

@@ -6,7 +6,7 @@
  * see pb_encode.h or pb_decode.h
  */
 
-#define NANOPB_VERSION nanopb-0.2.1
+#define NANOPB_VERSION nanopb-0.2.1-dev
 
 #ifdef PB_SYSTEM_HEADER
 #include PB_SYSTEM_HEADER
@@ -30,7 +30,7 @@
 #   define PB_PACKED_STRUCT_START _Pragma("pack(push, 1)")
 #   define PB_PACKED_STRUCT_END _Pragma("pack(pop)")
 #   define pb_packed
-#elif defined(_MSC_VER) && (_MSC_VER >= 1500)
+#elif defined(_MSC_VER)
     /* For Microsoft Visual C++ */
 #   define PB_PACKED_STRUCT_START __pragma(pack(push, 1))
 #   define PB_PACKED_STRUCT_END __pragma(pack(pop))

pb_decode.c

@@ -331,19 +331,6 @@ static bool pb_field_next(pb_field_iterator_t *iter)
     return notwrapped;
 }
 
-static bool checkreturn pb_field_find(pb_field_iterator_t *iter, uint32_t tag)
-{
-    unsigned start = iter->field_index;
-    
-    do {
-        if (iter->pos->tag == tag)
-            return true;
-        pb_field_next(iter);
-    } while (iter->field_index != start);
-    
-    return false;
-}
-
 /*************************
  * Decode a single field *
  *************************/
@@ -430,11 +417,11 @@ static bool checkreturn decode_callback_field(pb_istream_t *stream, pb_wire_type
         if (!pb_make_string_substream(stream, &substream))
             return false;
         
-        do
+        while (substream.bytes_left)
         {
             if (!pCallback->funcs.decode(&substream, iter->pos, arg))
                 PB_RETURN_ERROR(stream, "callback failed");
-        } while (substream.bytes_left);
+        }
         
         pb_close_string_substream(stream, &substream);
         return true;
@@ -472,71 +459,105 @@ static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_t
     }
 }
 
-/* Initialize message fields to default values, recursively */
+/* Set field count to zero (or clear has_ field). */
-static void pb_message_set_to_defaults(const pb_field_t fields[], void *dest_struct)
+static void pb_clear_field_count(const pb_field_iterator_t *iter)
 {
-    pb_field_iterator_t iter;
+    pb_type_t type;
-    pb_field_init(&iter, fields, dest_struct);
+    type = iter->pos->type;
     
-    /* Initialize size/has fields and apply default values */
+    if (iter->pos->tag == 0)
-    do
+        return; /* Empty message type */
+
+    if (PB_ATYPE(type) == PB_ATYPE_STATIC)
     {
-        pb_type_t type;
+        if (PB_HTYPE(type) == PB_HTYPE_OPTIONAL)
-        type = iter.pos->type;
-    
-        if (iter.pos->tag == 0)
-            continue;
-        
-        if (PB_ATYPE(type) == PB_ATYPE_STATIC)
         {
-            /* Initialize the size field for optional/repeated fields to 0. */
+            *(bool*)iter->pSize = false;
-            if (PB_HTYPE(type) == PB_HTYPE_OPTIONAL)
-            {
-                *(bool*)iter.pSize = false;
-            }
-            else if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
-            {
-                *(size_t*)iter.pSize = 0;
-                continue; /* Array is empty, no need to initialize contents */
-            }
-            
-            /* Initialize field contents to default value */
-            if (PB_LTYPE(iter.pos->type) == PB_LTYPE_SUBMESSAGE)
-            {
-                pb_message_set_to_defaults((const pb_field_t *) iter.pos->ptr, iter.pData);
-            }
-            else if (iter.pos->ptr != NULL)
-            {
-                memcpy(iter.pData, iter.pos->ptr, iter.pos->data_size);
-            }
-            else
-            {
-                memset(iter.pData, 0, iter.pos->data_size);
-            }
         }
-        else if (PB_ATYPE(type) == PB_ATYPE_CALLBACK)
+        else if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
         {
-            continue; /* Don't overwrite callback */
+            *(size_t*)iter->pSize = 0;
         }
-    } while (pb_field_next(&iter));
+    }
+}
+
+/* Initialize message field to default value. Recurses on submessages. */
+static void pb_set_field_to_default(const pb_field_iterator_t *iter)
+{
+    pb_type_t type;
+    type = iter->pos->type;
+
+    if (iter->pos->tag == 0)
+        return; /* Empty message type */
+
+    /* We only need to initialize static fields.
+     * Furthermore, arrays do not need to be initialized as their length
+     * will be zero by default.
+     */
+    if (PB_ATYPE(type) == PB_ATYPE_STATIC &&
+        PB_HTYPE(type) != PB_HTYPE_REPEATED)
+    {
+        if (PB_LTYPE(iter->pos->type) == PB_LTYPE_SUBMESSAGE)
+        {
+            /* Submessage: initialize the fields recursively */
+            pb_field_iterator_t subiter;
+            pb_field_init(&subiter, (const pb_field_t *)iter->pos->ptr, iter->pData);
+            do {
+                pb_clear_field_count(&subiter);
+                pb_set_field_to_default(&subiter);
+            } while (pb_field_next(&subiter));
+        }
+        else if (iter->pos->ptr != NULL)
+        {
+            /* Normal field: copy the default value */
+            memcpy(iter->pData, iter->pos->ptr, iter->pos->data_size);
+        }
+        else
+        {
+            /* Normal field without default value: initialize to zero */
+            memset(iter->pData, 0, iter->pos->data_size);
+        }
+    }
 }
 
 /*********************
  * Decode all fields *
  *********************/
 
-bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct)
+/* Helper function to initialize fields while advancing iterator */
+static void advance_iterator(pb_field_iterator_t *iter, bool *initialize, bool *current_seen)
+{
+    /* Initialize the fields we didn't decode. */
+    if (*initialize && !*current_seen)
+        pb_set_field_to_default(iter);
+    
+    /* Stop initializing after the first pass through the array */
+    if (!pb_field_next(iter))
+        *initialize = false;
+    
+    /* Clear the field count before decoding */
+    if (*initialize)
+        pb_clear_field_count(iter);
+    
+    /* Reset the flag to indicate that the new field has not been written to yet. */
+    *current_seen = false;
+}
+
+static bool checkreturn pb_decode_inner(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct, bool initialize)
 {
     uint8_t fields_seen[(PB_MAX_REQUIRED_FIELDS + 7) / 8] = {0}; /* Used to check for required fields */
+    bool current_seen = false;
     pb_field_iterator_t iter;
-    
     pb_field_init(&iter, fields, dest_struct);
+    pb_clear_field_count(&iter);
     
     while (stream->bytes_left)
     {
         uint32_t tag;
         pb_wire_type_t wire_type;
         bool eof;
+        unsigned start;
+        bool skip = false;
         
         if (!pb_decode_tag(stream, &wire_type, &tag, &eof))
         {
@@ -546,20 +567,45 @@ bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[
                 return false;
         }
         
-        if (!pb_field_find(&iter, tag))
+        /* Go through the fields until we either find a match or
+         * wrap around to start. On the first pass, also initialize
+         * any missing fields.
+         * 
+         * The logic here is to avoid unnecessary initialization
+         * in the common case, where all fields occur in the proper
+         * order.
+         */
+        start = iter.field_index;
+        while (iter.pos->tag != tag)
+        {
+            advance_iterator(&iter, &initialize, &current_seen);
+            
+            if (iter.field_index == start)
+            {
+                skip = true;
+                break;
+            }
+        }
+        
+        /* Skip data if field was not found */
+        if (skip)
         {
-            /* No match found, skip data */
             if (!pb_skip_field(stream, wire_type))
                 return false;
+
             continue;
         }
         
+        current_seen = true;
+        
+        /* Keep track if all required fields are present */
         if (PB_HTYPE(iter.pos->type) == PB_HTYPE_REQUIRED
             && iter.required_field_index < PB_MAX_REQUIRED_FIELDS)
         {
             fields_seen[iter.required_field_index >> 3] |= (uint8_t)(1 << (iter.required_field_index & 7));
         }
             
+        /* Finally, decode the field data */
         if (!decode_field(stream, wire_type, &iter))
             return false;
     }
@@ -569,6 +615,9 @@ bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[
         /* First figure out the number of required fields by
          * seeking to the end of the field array. Usually we
          * are already close to end after decoding.
+         *
+         * Note: this simultaneously initializes any fields
+         * that haven't been already initialized.
          */
         unsigned req_field_count;
         pb_type_t last_type;
@@ -576,7 +625,8 @@ bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[
         do {
             req_field_count = iter.required_field_index;
             last_type = iter.pos->type;
-        } while (pb_field_next(&iter));
+            advance_iterator(&iter, &initialize, &current_seen);
+        } while (iter.field_index != 0);
         
         /* Fixup if last field was also required. */
         if (PB_HTYPE(last_type) == PB_HTYPE_REQUIRED && iter.pos->tag)
@@ -597,10 +647,14 @@ bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[
     return true;
 }
 
+bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct)
+{
+    return pb_decode_inner(stream, fields, dest_struct, false);
+}
+
 bool checkreturn pb_decode(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct)
 {
-    pb_message_set_to_defaults(fields, dest_struct);
+    return pb_decode_inner(stream, fields, dest_struct, true);
-    return pb_decode_noinit(stream, fields, dest_struct);
 }
 
 /* Field decoders */
@@ -664,8 +718,7 @@ bool pb_decode_fixed64(pb_istream_t *stream, void *dest)
 bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
     uint64_t value;
-    if (!pb_decode_varint(stream, &value))
+    bool status = pb_decode_varint(stream, &value);
-        return false;
     
     switch (field->data_size)
     {
@@ -676,14 +729,13 @@ bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, vo
         default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
     
-    return true;
+    return status;
 }
 
 bool checkreturn pb_dec_svarint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
     int64_t value;
-    if (!pb_decode_svarint(stream, &value))
+    bool status = pb_decode_svarint(stream, &value);
-        return false;
     
     switch (field->data_size)
     {
@@ -692,7 +744,7 @@ bool checkreturn pb_dec_svarint(pb_istream_t *stream, const pb_field_t *field, v
         default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
     
-    return true;
+    return status;
 }
 
 bool checkreturn pb_dec_fixed32(pb_istream_t *stream, const pb_field_t *field, void *dest)
@@ -751,12 +803,7 @@ bool checkreturn pb_dec_submessage(pb_istream_t *stream, const pb_field_t *field
     if (field->ptr == NULL)
         PB_RETURN_ERROR(stream, "invalid field descriptor");
     
-    /* New array entries need to be initialized, while required and optional
+    status = pb_decode(&substream, submsg_fields, dest);
-     * submessages have already been initialized in the top-level pb_decode. */
-    if (PB_HTYPE(field->type) == PB_HTYPE_REPEATED)
-        status = pb_decode(&substream, submsg_fields, dest);
-    else
-        status = pb_decode_noinit(&substream, submsg_fields, dest);
     
     pb_close_string_substream(stream, &substream);
     return status;

pb_decode.h

@@ -9,6 +9,7 @@
  * These are usually generated from .proto-files with a script.
  */
 
+#include <stdbool.h>
 #include "pb.h"
 
 #ifdef __cplusplus
@@ -61,6 +62,9 @@ bool pb_decode(pb_istream_t *stream, const pb_field_t fields[], void *dest_struc
 /* Same as pb_decode, except does not initialize the destination structure
  * to default values. This is slightly faster if you need no default values
  * and just do memset(struct, 0, sizeof(struct)) yourself.
+ *
+ * It can also be used to merge fields from a new message into a previously
+ * initialized structure.
  */
 bool pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct);
 

pb_encode.c

@@ -461,16 +461,8 @@ bool checkreturn pb_enc_bytes(pb_ostream_t *stream, const pb_field_t *field, con
 
 bool checkreturn pb_enc_string(pb_ostream_t *stream, const pb_field_t *field, const void *src)
 {
-    /* strnlen() is not always available, so just use a for-loop */
+    UNUSED(field);
-    size_t size = 0;
+    return pb_encode_string(stream, (const uint8_t*)src, strlen((const char*)src));
-    const char *p = (const char*)src;
-    while (size < field->data_size && *p != '\0')
-    {
-        size++;
-        p++;
-    }
-
-    return pb_encode_string(stream, (const uint8_t*)src, size);
 }
 
 bool checkreturn pb_enc_submessage(pb_ostream_t *stream, const pb_field_t *field, const void *src)

pb_encode.h

@@ -6,6 +6,7 @@
  * and their field descriptions (just like with pb_decode).
  */
 
+#include <stdbool.h>
 #include "pb.h"
 
 #ifdef __cplusplus

tests/callbacks.proto

@@ -11,6 +11,5 @@ message TestMessage {
     repeated fixed32 fixed32value = 3;
     repeated fixed64 fixed64value = 4;
     optional SubMessage submsg = 5;
-    repeated string repeatedstring = 6;
 }
 

tests/encode_unittests.c

@@ -180,14 +180,12 @@ int main()
     {
         uint8_t buffer[30];
         pb_ostream_t s;
-        char value[30] = "xyzzy";
+        char value[] = "xyzzy";
         
         COMMENT("Test pb_enc_string")
-        TEST(WRITES(pb_enc_string(&s, &StringMessage_fields[0], &value), "\x05xyzzy"))
+        TEST(WRITES(pb_enc_string(&s, NULL, &value), "\x05xyzzy"))
         value[0] = '\0';
-        TEST(WRITES(pb_enc_string(&s, &StringMessage_fields[0], &value), "\x00"))
+        TEST(WRITES(pb_enc_string(&s, NULL, &value), "\x00"))
-        memset(value, 'x', 30);
-        TEST(WRITES(pb_enc_string(&s, &StringMessage_fields[0], &value), "\x0Axxxxxxxxxx"))
     }
     
     {

tests/test_decode_callbacks.c

@@ -83,8 +83,6 @@ int main()
     testmessage.fixed32value.arg = "fixed32value: %ld\n";
     testmessage.fixed64value.funcs.decode = &print_fixed64;
     testmessage.fixed64value.arg = "fixed64value: %lld\n";
-    testmessage.repeatedstring.funcs.decode = &print_string;
-    testmessage.repeatedstring.arg = "repeatedstring: \"%s\"\n";
     
     if (!pb_decode(&stream, TestMessage_fields, &testmessage))
         return 1;

tests/test_encode_callbacks.c

@@ -41,22 +41,6 @@ bool encode_fixed64(pb_ostream_t *stream, const pb_field_t *field, void * const
     return pb_encode_fixed64(stream, &value);
 }
 
-bool encode_repeatedstring(pb_ostream_t *stream, const pb_field_t *field, void * const *arg)
-{
-    char *str[4] = {"Hello world!", "", "Test", "Test2"};
-    int i;
-    
-    for (i = 0; i < 4; i++)
-    {
-        if (!pb_encode_tag_for_field(stream, field))
-            return false;
-        
-        if (!pb_encode_string(stream, (uint8_t*)str[i], strlen(str[i])))
-            return false;
-    }
-    return true;
-}
-
 int main()
 {
     uint8_t buffer[1024];
@@ -73,8 +57,6 @@ int main()
     testmessage.submsg.int32value.funcs.encode = &encode_int32;
     testmessage.submsg.fixed32value.funcs.encode = &encode_fixed32;
     testmessage.submsg.fixed64value.funcs.encode = &encode_fixed64;
-
-    testmessage.repeatedstring.funcs.encode = &encode_repeatedstring;
     
     if (!pb_encode(&stream, TestMessage_fields, &testmessage))
         return 1;

tests/unittestproto.proto

@@ -8,10 +8,6 @@ message FloatArray {
     repeated float data = 1 [(nanopb).max_count = 10];
 }
 
-message StringMessage {
-    required string data = 1 [(nanopb).max_size = 10];
-}
-
 message CallbackArray {
     // We cheat a bit and use this message for testing other types, too.
     // Nanopb does not care about the actual defined data type for callback