Switch pb_encode to use the common iterator logic in pb_common.c

Update issue 128
Status: FixedInGit

5 files changed, 42 insertions, 43 deletions

diff --git a/pb_common.c b/pb_common.c
index de217699..a9cade63 100644
--- a/pb_common.c
+++ b/pb_common.c
@@ -5,7 +5,7 @@
 
 #include "pb_common.h"
 
-void pb_field_iter_begin(pb_field_iter_t *iter, const pb_field_t *fields, void *dest_struct)
+bool pb_field_iter_begin(pb_field_iter_t *iter, const pb_field_t *fields, void *dest_struct)
 {
     iter->start = fields;
     iter->pos = fields;
@@ -13,6 +13,8 @@ void pb_field_iter_begin(pb_field_iter_t *iter, const pb_field_t *fields, void *
     iter->dest_struct = dest_struct;
     iter->pData = (char*)dest_struct + iter->pos->data_offset;
     iter->pSize = (char*)iter->pData + iter->pos->size_offset;
+    
+    return (iter->pos->tag != 0);
 }
 
 bool pb_field_iter_next(pb_field_iter_t *iter)
@@ -31,7 +33,7 @@ bool pb_field_iter_next(pb_field_iter_t *iter)
     if (iter->pos->tag == 0)
     {
         /* Wrapped back to beginning, reinitialize */
-        pb_field_iter_begin(iter, iter->start, iter->dest_struct);
+        (void)pb_field_iter_begin(iter, iter->start, iter->dest_struct);
         return false;
     }
     else
diff --git a/pb_common.h b/pb_common.h
index e85f000c..01a37682 100644
--- a/pb_common.h
+++ b/pb_common.h
@@ -16,13 +16,14 @@ typedef struct {
     const pb_field_t *start;       /* Start of the pb_field_t array */
     const pb_field_t *pos;         /* Current position of the iterator */
     unsigned required_field_index; /* Zero-based index that counts only the required fields */
-    void *dest_struct;             /* Pointer to the destination structure to decode to */
-    void *pData;                   /* Pointer where to store current field value */
-    void *pSize;                   /* Pointer where to store the size of current array field */
+    void *dest_struct;             /* Pointer to start of the structure */
+    void *pData;                   /* Pointer to current field value */
+    void *pSize;                   /* Pointer to count/has field */
 } pb_field_iter_t;
 
-/* Initialize the field iterator structure to beginning. */
-void pb_field_iter_begin(pb_field_iter_t *iter, const pb_field_t *fields, void *dest_struct);
+/* Initialize the field iterator structure to beginning.
+ * Returns false if the message type is empty. */
+bool pb_field_iter_begin(pb_field_iter_t *iter, const pb_field_t *fields, void *dest_struct);
 
 /* Advance the iterator to the next field.
  * Returns false when the iterator wraps back to the first field. */
diff --git a/pb_decode.c b/pb_decode.c
index 40da1aa4..3e817cc8 100644
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -616,7 +616,7 @@ static bool checkreturn default_extension_decoder(pb_istream_t *stream,
     /* Fake a field iterator for the extension field.
      * It is not actually safe to advance this iterator, but decode_field
      * will not even try to. */
-    pb_field_iter_begin(&iter, field, extension->dest);
+    (void)pb_field_iter_begin(&iter, field, extension->dest);
     iter.pData = extension->dest;
     iter.pSize = &extension->found;
     
@@ -668,16 +668,14 @@ static bool checkreturn find_extension_field(pb_field_iter_t *iter)
 static void pb_message_set_to_defaults(const pb_field_t fields[], void *dest_struct)
 {
     pb_field_iter_t iter;
-    pb_field_iter_begin(&iter, fields, dest_struct);
+
+    if (!pb_field_iter_begin(&iter, fields, dest_struct))
+        return; /* Empty message type */
     
     do
     {
         pb_type_t type;
         type = iter.pos->type;
-    
-        /* Avoid crash on empty message types (zero fields) */
-        if (iter.pos->tag == 0)
-            continue;
         
         if (PB_ATYPE(type) == PB_ATYPE_STATIC)
         {
@@ -738,7 +736,9 @@ bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[
     uint32_t extension_range_start = 0;
     pb_field_iter_t iter;
     
-    pb_field_iter_begin(&iter, fields, dest_struct);
+    /* Return value ignored, as empty message types will be correctly handled by
+     * pb_field_iter_find() anyway. */
+    (void)pb_field_iter_begin(&iter, fields, dest_struct);
     
     while (stream->bytes_left)
     {
@@ -859,17 +859,15 @@ bool pb_decode_delimited(pb_istream_t *stream, const pb_field_t fields[], void *
 void pb_release(const pb_field_t fields[], void *dest_struct)
 {
     pb_field_iter_t iter;
-    pb_field_iter_begin(&iter, fields, dest_struct);
+    
+    if (!pb_field_iter_begin(&iter, fields, dest_struct))
+        return; /* Empty message type */
     
     do
     {
         pb_type_t type;
         type = iter.pos->type;
     
-        /* Avoid crash on empty message types (zero fields) */
-        if (iter.pos->tag == 0)
-            continue;
-        
         if (PB_ATYPE(type) == PB_ATYPE_POINTER)
         {
             if (PB_HTYPE(type) == PB_HTYPE_REPEATED &&
diff --git a/pb_encode.c b/pb_encode.c
index dc5a2734..6041c641 100644
--- a/pb_encode.c
+++ b/pb_encode.c
@@ -5,6 +5,7 @@
 
 #include "pb.h"
 #include "pb_encode.h"
+#include "pb_common.h"
 
 /* Use the GCC warn_unused_result attribute to check that all return values
  * are propagated correctly. On other compilers and gcc before 3.4.0 just
@@ -333,42 +334,38 @@ static bool checkreturn encode_extension_field(pb_ostream_t *stream,
  * Encode all fields *
  *********************/
 
+static void *remove_const(const void *p)
+{
+    /* Note: this casts away const, in order to use the common field iterator
+     * logic for both encoding and decoding. */
+    union {
+        void *p1;
+        const void *p2;
+    } t;
+    t.p2 = p;
+    return t.p1;
+}
+
 bool checkreturn pb_encode(pb_ostream_t *stream, const pb_field_t fields[], const void *src_struct)
 {
-    const pb_field_t *field = fields;
-    const void *pData = src_struct;
-    size_t prev_size = 0;
+    pb_field_iter_t iter;
+    if (!pb_field_iter_begin(&iter, fields, remove_const(src_struct)))
+        return true; /* Empty message type */
     
-    while (field->tag != 0)
-    {
-        pData = (const char*)pData + prev_size + field->data_offset;
-        if (PB_ATYPE(field->type) == PB_ATYPE_POINTER)
-            prev_size = sizeof(const void*);
-        else
-            prev_size = field->data_size;
-        
-        /* Special case for static arrays */
-        if (PB_ATYPE(field->type) == PB_ATYPE_STATIC &&
-            PB_HTYPE(field->type) == PB_HTYPE_REPEATED)
-        {
-            prev_size *= field->array_size;
-        }
-        
-        if (PB_LTYPE(field->type) == PB_LTYPE_EXTENSION)
+    do {
+        if (PB_LTYPE(iter.pos->type) == PB_LTYPE_EXTENSION)
         {
             /* Special case for the extension field placeholder */
-            if (!encode_extension_field(stream, field, pData))
+            if (!encode_extension_field(stream, iter.pos, iter.pData))
                 return false;
         }
         else
         {
             /* Regular field */
-            if (!encode_field(stream, field, pData))
+            if (!encode_field(stream, iter.pos, iter.pData))
                 return false;
         }
-    
-        field++;
-    }
+    } while (pb_field_iter_next(&iter));
     
     return true;
 }
diff --git a/tests/splint/splint.rc b/tests/splint/splint.rc
index 421f5677..e2b26888 100644
--- a/tests/splint/splint.rc
+++ b/tests/splint/splint.rc
@@ -28,6 +28,7 @@
 -kepttrans
 -branchstate
 -immediatetrans
+-mustfreefresh
 
 # These tests give false positives, compiler typically has
 # better warnings for these.