diff --git a/objectivec/GPBUnknownField.m b/objectivec/GPBUnknownField.m index ba70dfe59930..f574acf7e062 100644 --- a/objectivec/GPBUnknownField.m +++ b/objectivec/GPBUnknownField.m @@ -165,9 +165,17 @@ - (id)copyWithZone:(NSZone *)zone { case GPBUnknownFieldTypeFixed32: case GPBUnknownFieldTypeFixed64: case GPBUnknownFieldTypeLengthDelimited: - case GPBUnknownFieldTypeGroup: // In these modes, the object isn't mutable, so just return self. return [self retain]; + case GPBUnknownFieldTypeGroup: { + // The `GPBUnknownFields` for the group is mutable, so a new instance of this object and + // the group is needed. + GPBUnknownFields *copyGroup = [storage_.group copyWithZone:zone]; + GPBUnknownField *copy = [[GPBUnknownField allocWithZone:zone] initWithNumber:number_ + group:copyGroup]; + [copyGroup release]; + return copy; + } case GPBUnknownFieldTypeLegacy: { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" diff --git a/objectivec/GPBUnknownFields.h b/objectivec/GPBUnknownFields.h index 07a7af5f5107..859614b6453a 100644 --- a/objectivec/GPBUnknownFields.h +++ b/objectivec/GPBUnknownFields.h @@ -109,6 +109,23 @@ __attribute__((objc_subclassing_restricted)) **/ - (nonnull GPBUnknownFields *)addGroupWithFieldNumber:(int32_t)fieldNumber; +/** + * Add the copy of the given unknown field. + * + * This can be useful from processing one `GPBUnknownFields` to create another. + * + * NOTE: If the field being copied is an Group, this instance added is new and thus + * the `.group` of that result is also new, so if you intent is to modify the group + * it *must* be fetched out of the result. + * + * It is a programming error to call this when the `type` is a legacy field. + * + * @param field The field to add. + * + * @return The autoreleased field that was added. + **/ +- (GPBUnknownField *)addCopyOfField:(nonnull GPBUnknownField *)field; + /** * Removes the given field from the set. * diff --git a/objectivec/GPBUnknownFields.m b/objectivec/GPBUnknownFields.m index 2672d10b15b1..dc004b3fb899 100644 --- a/objectivec/GPBUnknownFields.m +++ b/objectivec/GPBUnknownFields.m @@ -197,6 +197,12 @@ - (instancetype)initFromMessage:(nonnull GPBMessage *)message { self = [super init]; if (self) { fields_ = [[NSMutableArray alloc] init]; + // This shouldn't happen with the annotations, but just incase something claiming nonnull + // does return nil, block it. + if (!message) { + [self release]; + [NSException raise:NSInvalidArgumentException format:@"Message cannot be nil"]; + } NSData *data = GPBMessageUnknownFieldsData(message); if (data) { GPBCodedInputStream *input = [[GPBCodedInputStream alloc] initWithData:data]; @@ -222,9 +228,8 @@ - (instancetype)init { } - (id)copyWithZone:(NSZone *)zone { - GPBUnknownFields *copy = [[GPBUnknownFields alloc] init]; - // Fields are r/o in this api, so just copy the array. - copy->fields_ = [fields_ mutableCopyWithZone:zone]; + GPBUnknownFields *copy = [[GPBUnknownFields allocWithZone:zone] init]; + copy->fields_ = [[NSMutableArray allocWithZone:zone] initWithArray:fields_ copyItems:YES]; return copy; } @@ -318,6 +323,16 @@ - (GPBUnknownFields *)addGroupWithFieldNumber:(int32_t)fieldNumber { return [group autorelease]; } +- (GPBUnknownField *)addCopyOfField:(nonnull GPBUnknownField *)field { + if (field->type_ == GPBUnknownFieldTypeLegacy) { + [NSException raise:NSInternalInconsistencyException + format:@"GPBUnknownField is the wrong type"]; + } + GPBUnknownField *result = [field copy]; + [fields_ addObject:result]; + return [result autorelease]; +} + - (void)removeField:(nonnull GPBUnknownField *)field { NSUInteger count = fields_.count; [fields_ removeObjectIdenticalTo:field]; diff --git a/objectivec/Tests/GPBUnknownFieldsTest.m b/objectivec/Tests/GPBUnknownFieldsTest.m index 42c3f7dc87e6..2b8a085072e0 100644 --- a/objectivec/Tests/GPBUnknownFieldsTest.m +++ b/objectivec/Tests/GPBUnknownFieldsTest.m @@ -662,6 +662,37 @@ - (void)testFastEnumeration { XCTAssertEqual(loop, 10); } +- (void)testAddCopyOfField { + GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; + [ufs addFieldNumber:1 varint:10]; + [ufs addFieldNumber:2 fixed32:11]; + [ufs addFieldNumber:3 fixed64:12]; + [ufs addFieldNumber:4 lengthDelimited:DataFromCStr("foo")]; + GPBUnknownFields* group = [ufs addGroupWithFieldNumber:5]; + [group addFieldNumber:10 varint:100]; + GPBUnknownFields* subGroup = [group addGroupWithFieldNumber:100]; + [subGroup addFieldNumber:50 varint:50]; + + GPBUnknownFields* ufs2 = [[[GPBUnknownFields alloc] init] autorelease]; + for (GPBUnknownField* field in ufs) { + GPBUnknownField* field2 = [ufs2 addCopyOfField:field]; + XCTAssertEqualObjects(field, field2); + if (field.type == GPBUnknownFieldTypeGroup) { + // Group does a copy because the `.group` value is mutable. + XCTAssertTrue(field != field2); // Pointer comparison. + XCTAssertTrue(group != field2.group); // Pointer comparison. + XCTAssertEqualObjects(group, field2.group); + GPBUnknownFields* subGroupAdded = [field2.group firstGroup:100]; + XCTAssertTrue(subGroupAdded != subGroup); // Pointer comparison. + XCTAssertEqualObjects(subGroupAdded, subGroup); + } else { + // All other types are immutable, so they use the same object. + XCTAssertTrue(field == field2); // Pointer comparision. + } + } + XCTAssertEqualObjects(ufs, ufs2); +} + - (void)testDescriptions { // Exercise description for completeness. GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; @@ -683,25 +714,30 @@ - (void)testCopy { [ufs addFieldNumber:3 fixed64:3]; [ufs addFieldNumber:4 lengthDelimited:DataFromCStr("foo")]; GPBUnknownFields* group = [ufs addGroupWithFieldNumber:5]; + [group addFieldNumber:10 varint:10]; + GPBUnknownFields* subGroup = [group addGroupWithFieldNumber:100]; + [subGroup addFieldNumber:20 varint:20]; GPBUnknownFields* ufs2 = [[ufs copy] autorelease]; XCTAssertTrue(ufs != ufs2); // Different objects XCTAssertEqualObjects(ufs, ufs2); // Equal contents - // All the actual field objects should be the same since they are immutable. + // All field objects but the group should be the same since they are immutable. XCTAssertTrue([[ufs fields:1] firstObject] == [[ufs2 fields:1] firstObject]); // Same object XCTAssertTrue([[ufs fields:2] firstObject] == [[ufs2 fields:2] firstObject]); // Same object XCTAssertTrue([[ufs fields:3] firstObject] == [[ufs2 fields:3] firstObject]); // Same object XCTAssertTrue([[ufs fields:4] firstObject] == [[ufs2 fields:4] firstObject]); // Same object XCTAssertTrue([[ufs fields:4] firstObject].lengthDelimited == - [[ufs2 fields:4] firstObject].lengthDelimited); // Same object - XCTAssertTrue([[ufs fields:5] firstObject] == [[ufs2 fields:5] firstObject]); // Same object - XCTAssertTrue(group == [[ufs2 fields:5] firstObject].group); // Same object - - // Now force copies on the fields to confirm that is not making new objects either. - for (GPBUnknownField* field in ufs) { - GPBUnknownField* field2 = [[field copy] autorelease]; - XCTAssertTrue(field == field2); // Same object (since they aren't mutable). - } + [[ufs2 fields:4] firstObject].lengthDelimited); // Same object + // Since the group holds another `GPBUnknownFields` object (which is mutable), it will be a + // different object. + XCTAssertTrue([[ufs fields:5] firstObject] != [[ufs2 fields:5] firstObject]); + XCTAssertTrue(group != [[ufs2 fields:5] firstObject].group); + XCTAssertEqualObjects(group, [[ufs2 fields:5] firstObject].group); + // And confirm that copy went deep so the nested group also is a different object. + GPBUnknownFields* groupCopied = [[ufs2 fields:5] firstObject].group; + XCTAssertTrue([[group fields:100] firstObject] != [[groupCopied fields:100] firstObject]); + XCTAssertTrue(subGroup != [[groupCopied fields:100] firstObject].group); + XCTAssertEqualObjects(subGroup, [[groupCopied fields:100] firstObject].group); } - (void)testInvalidFieldNumbers {