Copy the value when setting message/data fields.

Follow ObjC conventions and how the generated header labels things by copying NSStrings/NSData fields when setting them.
gminor7 · Oct 2, 2018 · 09c001e · 09c001e
1 parent 97d03ab
commit 09c001e
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 6 deletions.
diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m
@@ -3084,6 +3084,14 @@ static void ResolveIvarSet(__unsafe_unretained GPBFieldDescriptor *field,
       result->encodingSelector = @selector(set##NAME:);                       \
       break;                                                                  \
     }
+#define CASE_SET_COPY(NAME)                                                   \
+    case GPBDataType##NAME: {                                                 \
+      result->impToAdd = imp_implementationWithBlock(^(id obj, id value) {    \
+        return GPBSetRetainedObjectIvarWithFieldInternal(obj, field, [value copy], syntax); \
+      });                                                                     \
+      result->encodingSelector = @selector(set##NAME:);                       \
+      break;                                                                  \
+    }
       CASE_SET(Bool, BOOL, Bool)
       CASE_SET(Fixed32, uint32_t, UInt32)
       CASE_SET(SFixed32, int32_t, Int32)
@@ -3097,8 +3105,8 @@ static void ResolveIvarSet(__unsafe_unretained GPBFieldDescriptor *field,
       CASE_SET(SInt64, int64_t, Int64)
       CASE_SET(UInt32, uint32_t, UInt32)
       CASE_SET(UInt64, uint64_t, UInt64)
-      CASE_SET(Bytes, id, Object)
-      CASE_SET(String, id, Object)
+      CASE_SET_COPY(Bytes)
+      CASE_SET_COPY(String)
       CASE_SET(Message, id, Object)
       CASE_SET(Group, id, Object)
       CASE_SET(Enum, int32_t, Enum)

diff --git a/objectivec/GPBUtilities.m b/objectivec/GPBUtilities.m
@@ -448,6 +448,36 @@ void GPBMaybeClearOneof(GPBMessage *self, GPBOneofDescriptor *oneof,
 //%  GPBSetObjectIvarWithField(self, field, (id)value);
 //%}
 //%
+//%PDDM-DEFINE IVAR_ALIAS_DEFN_COPY_OBJECT(NAME, TYPE)
+//%// Only exists for public api, no core code should use this.
+//%TYPE *GPBGetMessage##NAME##Field(GPBMessage *self,
+//% TYPE$S             NAME$S       GPBFieldDescriptor *field) {
+//%#if defined(DEBUG) && DEBUG
+//%  NSCAssert(DataTypesEquivalent(GPBGetFieldDataType(field),
+//%                                GPBDataType##NAME),
+//%            @"Attempting to get value of TYPE from field %@ "
+//%            @"of %@ which is of type %@.",
+//%            [self class], field.name,
+//%            TypeToString(GPBGetFieldDataType(field)));
+//%#endif
+//%  return (TYPE *)GPBGetObjectIvarWithField(self, field);
+//%}
+//%
+//%// Only exists for public api, no core code should use this.
+//%void GPBSetMessage##NAME##Field(GPBMessage *self,
+//%                   NAME$S     GPBFieldDescriptor *field,
+//%                   NAME$S     TYPE *value) {
+//%#if defined(DEBUG) && DEBUG
+//%  NSCAssert(DataTypesEquivalent(GPBGetFieldDataType(field),
+//%                                GPBDataType##NAME),
+//%            @"Attempting to set field %@ of %@ which is of type %@ with "
+//%            @"value of type TYPE.",
+//%            [self class], field.name,
+//%            TypeToString(GPBGetFieldDataType(field)));
+//%#endif
+//%  GPBSetCopyObjectIvarWithField(self, field, (id)value);
+//%}
+//%
 
 // Object types are handled slightly differently, they need to be released
 // and retained.
@@ -483,6 +513,24 @@ static void GPBSetObjectIvarWithField(GPBMessage *self,
                                             syntax);
 }
 
+static void GPBSetCopyObjectIvarWithField(GPBMessage *self,
+                                          GPBFieldDescriptor *field, id value);
+
+// GPBSetCopyObjectIvarWithField is blocked from the analyzer because it flags
+// a leak for the -copy even though GPBSetRetainedObjectIvarWithFieldInternal
+// is marked as consuming the value. Note: For some reason this doesn't happen
+// with the -retain in GPBSetObjectIvarWithField.
+#if !defined(__clang_analyzer__)
+// This exists only for briging some aliased types, nothing else should use it.
+static void GPBSetCopyObjectIvarWithField(GPBMessage *self,
+                                          GPBFieldDescriptor *field, id value) {
+  if (self == nil || field == nil) return;
+  GPBFileSyntax syntax = [self descriptor].file.syntax;
+  GPBSetRetainedObjectIvarWithFieldInternal(self, field, [value copy],
+                                            syntax);
+}
+#endif  // !defined(__clang_analyzer__)
+
 void GPBSetObjectIvarWithFieldInternal(GPBMessage *self,
                                        GPBFieldDescriptor *field, id value,
                                        GPBFileSyntax syntax) {
@@ -1168,7 +1216,7 @@ void GPBSetDoubleIvarWithFieldInternal(GPBMessage *self,
 
 // Aliases are function calls that are virtually the same.
 
-//%PDDM-EXPAND IVAR_ALIAS_DEFN_OBJECT(String, NSString)
+//%PDDM-EXPAND IVAR_ALIAS_DEFN_COPY_OBJECT(String, NSString)
 // This block of code is generated, do not edit it directly.
 
 // Only exists for public api, no core code should use this.
@@ -1197,10 +1245,10 @@ void GPBSetMessageStringField(GPBMessage *self,
             [self class], field.name,
             TypeToString(GPBGetFieldDataType(field)));
 #endif
-  GPBSetObjectIvarWithField(self, field, (id)value);
+  GPBSetCopyObjectIvarWithField(self, field, (id)value);
 }
 
-//%PDDM-EXPAND IVAR_ALIAS_DEFN_OBJECT(Bytes, NSData)
+//%PDDM-EXPAND IVAR_ALIAS_DEFN_COPY_OBJECT(Bytes, NSData)
 // This block of code is generated, do not edit it directly.
 
 // Only exists for public api, no core code should use this.
@@ -1229,7 +1277,7 @@ void GPBSetMessageBytesField(GPBMessage *self,
             [self class], field.name,
             TypeToString(GPBGetFieldDataType(field)));
 #endif
-  GPBSetObjectIvarWithField(self, field, (id)value);
+  GPBSetCopyObjectIvarWithField(self, field, (id)value);
 }
 
 //%PDDM-EXPAND IVAR_ALIAS_DEFN_OBJECT(Message, GPBMessage)