Address review comments around NSNumber

ksperling-apple · ksperling-apple · commit 029bc93f0e70 · 2024-05-09T10:04:16.000+12:00
diff --git a/src/darwin/Framework/CHIP/MTRSetupPayload.h b/src/darwin/Framework/CHIP/MTRSetupPayload.h
@@ -61,18 +61,26 @@ typedef NS_ENUM(NSUInteger, MTROptionalQRCodeInfoType) {
 MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1))
 @interface MTROptionalQRCodeInfo : NSObject /* <NSCopying> (see below) */
 
-- (instancetype)initWithTag:(uint8_t)tag stringValue:(NSString *)value MTR_NEWLY_AVAILABLE;
-- (instancetype)initWithTag:(uint8_t)tag int32Value:(int32_t)value MTR_NEWLY_AVAILABLE;
+/**
+ * Initializes the object with a tag and string value.
+ * The tag must be in the range 0x80 - 0xFF.
+ */
+- (instancetype)initWithTag:(NSNumber *)tag stringValue:(NSString *)value MTR_NEWLY_AVAILABLE;
+
+/**
+ * Initializes the object with a tag and int32 value.
+ * The tag must be in the range 0x80 - 0xFF.
+ */
+- (instancetype)initWithTag:(NSNumber *)tag int32Value:(int32_t)value MTR_NEWLY_AVAILABLE;
 
 @property (nonatomic, readonly, assign) MTROptionalQRCodeInfoType type MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4));
 
 /**
- * The TLV tag number for this information item.
+ * The vendor-specific TLV tag number for this information item.
  *
- * Tags in the range 0x00 - 0x7F are reserved for Matter-defined elements.
- * Vendor-specific elements must have tags in the range 0x80 - 0xFF.
+ * Vendor-specific elements have tags in the range 0x80 - 0xFF.
  */
-@property (nonatomic, readonly, assign) uint8_t tagNumber MTR_NEWLY_AVAILABLE;
+@property (nonatomic, readonly, copy) NSNumber * tag;
 
 /**
  * The value held in this extension element,
@@ -160,18 +168,19 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1))
 @property (nonatomic, readonly, copy) NSArray<MTROptionalQRCodeInfo *> * vendorElements MTR_NEWLY_AVAILABLE;
 
 /**
- Returns the Manufacturer-specific extension element with the specified tag, if any.
+ * Returns the Manufacturer-specific extension element with the specified tag, if any.
+ * The tag must be in the range 0x80 - 0xFF.
  */
-- (nullable MTROptionalQRCodeInfo *)vendorElementWithTag:(uint8_t)tag MTR_NEWLY_AVAILABLE;
+- (nullable MTROptionalQRCodeInfo *)vendorElementWithTag:(NSNumber *)tag MTR_NEWLY_AVAILABLE;
 
 /**
  * Removes the extension element with the specified tag, if any.
+ * The tag must be in the range 0x80 - 0xFF.
  */
-- (void)removeVendorElementWithTag:(uint8_t)tag MTR_NEWLY_AVAILABLE;
+- (void)removeVendorElementWithTag:(NSNumber *)tag MTR_NEWLY_AVAILABLE;
 
 /**
  * Adds or replaces a Manufacturer-specific extension element.
- * The element must have a tag in the vendor-specific range (0x80 - 0xFF).
  */
 - (void)addOrReplaceVendorElement:(MTROptionalQRCodeInfo *)element MTR_NEWLY_AVAILABLE;
 
@@ -230,8 +239,6 @@ MTR_NEWLY_AVAILABLE
 @property (nonatomic, copy) NSNumber * infoType
     MTR_DEPRECATED("Please use type", ios(16.1, 16.4), macos(13.0, 13.3), watchos(9.1, 9.4), tvos(16.1, 16.4));
 
-@property (nonatomic, copy) NSNumber * tag MTR_NEWLY_DEPRECATED("Please use tagNumber");
-
 - (void)setType:(MTROptionalQRCodeInfoType)type MTR_NEWLY_DEPRECATED("MTROptionalQRCodeInfo is immutable");
 - (void)setTag:(NSNumber *)tag MTR_NEWLY_DEPRECATED("MTROptionalQRCodeInfo is immutable");
 - (void)setIntegerValue:(NSNumber *)integerValue MTR_NEWLY_DEPRECATED("MTROptionalQRCodeInfo is immutable");
diff --git a/src/darwin/Framework/CHIP/MTRSetupPayload.mm b/src/darwin/Framework/CHIP/MTRSetupPayload.mm
@@ -22,6 +22,7 @@
 #import "MTRLogging_Internal.h"
 #import "MTRUtilities.h"
 
+#include <lib/support/SafeInt.h>
 #include <setup_payload/ManualSetupPayloadGenerator.h>
 #include <setup_payload/ManualSetupPayloadParser.h>
 #include <setup_payload/QRCodeSetupPayloadGenerator.h>
@@ -34,20 +35,29 @@ @implementation MTROptionalQRCodeInfo {
     chip::OptionalQRCodeInfo _info;
 }
 
-- (instancetype)initWithTag:(uint8_t)tag int32Value:(int32_t)value
+static uint8_t ValidateVendorTag(NSNumber * tag)
+{
+    auto integerValue = tag.integerValue;
+    auto tagValue = static_cast<uint8_t>(integerValue);
+    MTRVerifyArgumentOrDie(tag != nil && chip::CanCastTo<uint8_t>(integerValue) && chip::SetupPayload::IsVendorTag(tagValue), @"tag must be a vendor tag (0x80 - 0xFF)");
+    return tagValue;
+}
+
+- (instancetype)initWithTag:(NSNumber *)tag int32Value:(int32_t)value
 {
     self = [super init];
     _info.type = chip::optionalQRCodeInfoTypeInt32;
-    _info.tag = tag;
+    _info.tag = ValidateVendorTag(tag);
     _info.int32 = value;
     return self;
 }
 
-- (instancetype)initWithTag:(uint8_t)tag stringValue:(NSString *)value
+- (instancetype)initWithTag:(NSNumber *)tag stringValue:(NSString *)value
 {
     self = [super init];
     _info.type = chip::optionalQRCodeInfoTypeString;
-    _info.tag = tag;
+    _info.tag = ValidateVendorTag(tag);
+    MTRVerifyArgumentOrDie(value != nil, @"value");
     _info.data = value.UTF8String;
     return self;
 }
@@ -56,7 +66,8 @@ - (nullable instancetype)initWithQRCodeInfo:(chip::OptionalQRCodeInfo const &)in
 {
     self = [super init];
     _info = info;
-    // Don't expose objects with an invalid type.
+    // Don't expose objects with an out-of-range tag or invalid type
+    VerifyOrReturnValue(chip::SetupPayload::IsVendorTag(_info.tag), nil);
     VerifyOrReturnValue(self.type != MTROptionalQRCodeInfoTypeUnknown, nil);
     return self;
 }
@@ -97,9 +108,9 @@ - (MTROptionalQRCodeInfoType)type
     return MTROptionalQRCodeInfoTypeUnknown;
 }
 
-- (uint8_t)tagNumber
+- (NSNumber *)tag
 {
-    return _info.tag;
+    return @(_info.tag);
 }
 
 - (NSNumber *)integerValue
@@ -142,7 +153,7 @@ - (BOOL)isEqual:(id)object
 
 - (NSString *)description
 {
-    return [NSString stringWithFormat:@"0x%02x=%@", self.tagNumber, self.integerValue ?: self.stringValue];
+    return [NSString stringWithFormat:@"0x%02x=%@", self.tag.unsignedCharValue, self.integerValue ?: self.stringValue];
 }
 
 @end
@@ -431,21 +442,21 @@ - (void)setSerialNumber:(nullable NSString *)serialNumber
     return infos;
 }
 
-- (MTROptionalQRCodeInfo *)vendorElementWithTag:(uint8_t)tag
+- (MTROptionalQRCodeInfo *)vendorElementWithTag:(NSNumber *)tag
 {
     chip::OptionalQRCodeInfo element;
-    VerifyOrReturnValue(_payload.getOptionalVendorData(tag, element) == CHIP_NO_ERROR, nil);
+    VerifyOrReturnValue(_payload.getOptionalVendorData(ValidateVendorTag(tag), element) == CHIP_NO_ERROR, nil);
     return [[MTROptionalQRCodeInfo alloc] initWithQRCodeInfo:element];
 }
 
-- (void)removeVendorElementWithTag:(uint8_t)tag
+- (void)removeVendorElementWithTag:(NSNumber *)tag
 {
-    _payload.removeOptionalVendorData(tag);
+    _payload.removeOptionalVendorData(ValidateVendorTag(tag));
 }
 
 - (void)addOrReplaceVendorElement:(MTROptionalQRCodeInfo *)element
 {
-    MTRVerifyArgumentOrDie(chip::SetupPayload::IsVendorTag(element.tagNumber), @"element.tagNumber");
+    MTRVerifyArgumentOrDie(element != nil, @"element");
     CHIP_ERROR err = [element addAsVendorElementTo:_payload];
     VerifyOrDieWithMsg(err == CHIP_NO_ERROR, NotSpecified, "Internal error: %" CHIP_ERROR_FORMAT, err.Format());
 }
@@ -615,19 +626,14 @@ @implementation MTROptionalQRCodeInfo (Deprecated)
 
 - (instancetype)init
 {
-    return [self initWithTag:0xff stringValue:@""];
+    return [self initWithTag:@0xff stringValue:@""];
 }
 
 - (void)setType:(MTROptionalQRCodeInfoType)type
 {
     /* ignored */
 }
 
-- (NSNumber *)tag
-{
-    return @(self.tagNumber);
-}
-
 - (void)setTag:(NSNumber *)tag
 {
     /* ignored */
diff --git a/src/darwin/Framework/CHIP/MTRSetupPayload_Internal.h b/src/darwin/Framework/CHIP/MTRSetupPayload_Internal.h
@@ -19,16 +19,11 @@
 
 #import "MTRDefines_Internal.h"
 
-#ifdef __cplusplus
-#import <lib/core/Optional.h>
 #import <setup_payload/SetupPayload.h>
-#endif
 
 MTR_DIRECT_MEMBERS
 @interface MTRSetupPayload ()
 
-#ifdef __cplusplus
 - (instancetype)initWithSetupPayload:(chip::SetupPayload)setupPayload;
-#endif
 
 @end
diff --git a/src/darwin/Framework/CHIPTests/MTRSetupPayloadTests.m b/src/darwin/Framework/CHIPTests/MTRSetupPayloadTests.m
@@ -212,49 +212,49 @@ - (void)testQRCodeParserWithOptionalData
     }
 
     // Test access by tag
-    XCTAssertEqualObjects([payload vendorElementWithTag:130].stringValue, @"myData");
-    XCTAssertEqualObjects([payload vendorElementWithTag:131].integerValue, @12);
+    XCTAssertEqualObjects([payload vendorElementWithTag:@130].stringValue, @"myData");
+    XCTAssertEqualObjects([payload vendorElementWithTag:@131].integerValue, @12);
 }
 
 - (void)testAddVendorElement
 {
     MTRSetupPayload * payload = [[MTRSetupPayload alloc] initWithSetupPasscode:@314159 discriminator:@555];
     XCTAssertEqual(payload.vendorElements.count, 0);
-    [payload addOrReplaceVendorElement:[[MTROptionalQRCodeInfo alloc] initWithTag:0xff int32Value:42]];
+    [payload addOrReplaceVendorElement:[[MTROptionalQRCodeInfo alloc] initWithTag:@0xff int32Value:42]];
     XCTAssertEqual(payload.vendorElements.count, 1);
     XCTAssertEqualObjects(payload.vendorElements.firstObject.integerValue, @42);
-    XCTAssertEqualObjects([payload vendorElementWithTag:0xff].integerValue, @42);
+    XCTAssertEqualObjects([payload vendorElementWithTag:@0xff].integerValue, @42);
 }
 
 - (void)testVendorElementsEncodedToQRCode
 {
     MTRSetupPayload * payload = [[MTRSetupPayload alloc] initWithSetupPasscode:@314159 discriminator:@555];
-    [payload addOrReplaceVendorElement:[[MTROptionalQRCodeInfo alloc] initWithTag:0x80 stringValue:@"Hello"]];
+    [payload addOrReplaceVendorElement:[[MTROptionalQRCodeInfo alloc] initWithTag:@0x80 stringValue:@"Hello"]];
     MTRSetupPayload * decoded = [[MTRSetupPayload alloc] initWithPayload:payload.qrCodeString];
     XCTAssertNotNil(decoded);
-    XCTAssertEqualObjects([decoded vendorElementWithTag:0x80].stringValue, @"Hello");
+    XCTAssertEqualObjects([decoded vendorElementWithTag:@0x80].stringValue, @"Hello");
 }
 
 - (void)testRemoveVendorElements
 {
     MTRSetupPayload * payload = [[MTRSetupPayload alloc] initWithPayload:@"MT:M5L90MP500K64J0A33P0SET70.QT52B.E23-WZE0WISA0DK5N1K8SQ1RYCU1O0"];
     XCTAssertNotNil(payload);
     XCTAssertEqual(payload.vendorElements.count, 2);
-    [payload removeVendorElementWithTag:0]; // no change, no vendor element present with this tag
+    [payload removeVendorElementWithTag:@128]; // no change, no vendor element present with this tag
     XCTAssertEqual(payload.vendorElements.count, 2);
-    [payload removeVendorElementWithTag:130];
+    [payload removeVendorElementWithTag:@130];
     XCTAssertEqual(payload.vendorElements.count, 1);
-    [payload removeVendorElementWithTag:131];
+    [payload removeVendorElementWithTag:@131];
     XCTAssertEqual(payload.vendorElements.count, 0);
 }
 
 - (void)testQrCodeInfoCopyAndEquality
 {
-    MTROptionalQRCodeInfo * a = [[MTROptionalQRCodeInfo alloc] initWithTag:1 stringValue:@"hello"];
-    MTROptionalQRCodeInfo * b = [[MTROptionalQRCodeInfo alloc] initWithTag:1 stringValue:@"hello"];
-    MTROptionalQRCodeInfo * c = [[MTROptionalQRCodeInfo alloc] initWithTag:0xff stringValue:@"hello"];
-    MTROptionalQRCodeInfo * d = [[MTROptionalQRCodeInfo alloc] initWithTag:1 int32Value:42];
-    MTROptionalQRCodeInfo * e = [[MTROptionalQRCodeInfo alloc] initWithTag:1 int32Value:0xbad];
+    MTROptionalQRCodeInfo * a = [[MTROptionalQRCodeInfo alloc] initWithTag:@0x88 stringValue:@"hello"];
+    MTROptionalQRCodeInfo * b = [[MTROptionalQRCodeInfo alloc] initWithTag:@0x88 stringValue:@"hello"];
+    MTROptionalQRCodeInfo * c = [[MTROptionalQRCodeInfo alloc] initWithTag:@0xff stringValue:@"hello"];
+    MTROptionalQRCodeInfo * d = [[MTROptionalQRCodeInfo alloc] initWithTag:@0x88 int32Value:42];
+    MTROptionalQRCodeInfo * e = [[MTROptionalQRCodeInfo alloc] initWithTag:@0x88 int32Value:0xbad];
     XCTAssertTrue([a isEqual:a]);
     XCTAssertTrue([a isEqual:[a copy]]);
     XCTAssertTrue([a isEqual:b]);
@@ -458,7 +458,7 @@ - (void)testCopyingAndEquality
     XCTAssertTrue([copy isEqual:payload]);
     XCTAssertEqual(payload.hash, copy.hash);
 
-    MTROptionalQRCodeInfo * element = [[MTROptionalQRCodeInfo alloc] initWithTag:0x80 stringValue:@"To infinity and beyond!"];
+    MTROptionalQRCodeInfo * element = [[MTROptionalQRCodeInfo alloc] initWithTag:@0x80 stringValue:@"To infinity and beyond!"];
     [copy addOrReplaceVendorElement:element];
     XCTAssertFalse([copy isEqual:payload]);
     [payload addOrReplaceVendorElement:element];
@@ -479,4 +479,12 @@ - (void)testCanParseFutureDiscoveryMethod
     XCTAssertEqual([[MTRSetupPayload alloc] initWithPayload:@"MT:-24J0Q.C.0KA0648G00"].discoveryCapabilities, 0xfa);
 }
 
+- (void)testDescriptionShowsUnknownDiscoveryMethods
+{
+    MTRSetupPayload * a = [[MTRSetupPayload alloc] initWithSetupPasscode:@888 discriminator:@555];
+    MTRSetupPayload * b = [a copy];
+    b.discoveryCapabilities |= 0x80;
+    XCTAssertNotEqualObjects(a.description, b.description);
+}
+
 @end

Original file line number	Diff line number	Diff line change
`@@ -22,6 +22,7 @@`
`22`	`22`	`#import "MTRLogging_Internal.h"`
`23`	`23`	`#import "MTRUtilities.h"`
`24`	`24`
	`25`	`+#include <lib/support/SafeInt.h>`
`25`	`26`	`#include <setup_payload/ManualSetupPayloadGenerator.h>`
`26`	`27`	`#include <setup_payload/ManualSetupPayloadParser.h>`
`27`	`28`	`#include <setup_payload/QRCodeSetupPayloadGenerator.h>`
`@@ -34,20 +35,29 @@ @implementation MTROptionalQRCodeInfo {`
`34`	`35`	`chip::OptionalQRCodeInfo _info;`
`35`	`36`	`}`
`36`	`37`
`37`		`-- (instancetype)initWithTag:(uint8_t)tag int32Value:(int32_t)value`
	`38`	`+static uint8_t ValidateVendorTag(NSNumber * tag)`
	`39`	`+{`
	`40`	`+ auto integerValue = tag.integerValue;`
	`41`	`+ auto tagValue = static_cast<uint8_t>(integerValue);`
	`42`	`+ MTRVerifyArgumentOrDie(tag != nil && chip::CanCastTo<uint8_t>(integerValue) && chip::SetupPayload::IsVendorTag(tagValue), @"tag must be a vendor tag (0x80 - 0xFF)");`
	`43`	`+ return tagValue;`
	`44`	`+}`
	`45`	`+`
	`46`	`+- (instancetype)initWithTag:(NSNumber *)tag int32Value:(int32_t)value`
`38`	`47`	`{`
`39`	`48`	`self = [super init];`
`40`	`49`	`_info.type = chip::optionalQRCodeInfoTypeInt32;`
`41`		`- _info.tag = tag;`
	`50`	`+ _info.tag = ValidateVendorTag(tag);`
`42`	`51`	`_info.int32 = value;`
`43`	`52`	`return self;`
`44`	`53`	`}`
`45`	`54`
`46`		`-- (instancetype)initWithTag:(uint8_t)tag stringValue:(NSString *)value`
	`55`	`+- (instancetype)initWithTag:(NSNumber )tag stringValue:(NSString )value`
`47`	`56`	`{`
`48`	`57`	`self = [super init];`
`49`	`58`	`_info.type = chip::optionalQRCodeInfoTypeString;`
`50`		`- _info.tag = tag;`
	`59`	`+ _info.tag = ValidateVendorTag(tag);`
	`60`	`+ MTRVerifyArgumentOrDie(value != nil, @"value");`
`51`	`61`	`_info.data = value.UTF8String;`
`52`	`62`	`return self;`
`53`	`63`	`}`
`@@ -56,7 +66,8 @@ - (nullable instancetype)initWithQRCodeInfo:(chip::OptionalQRCodeInfo const &)in`
`56`	`66`	`{`
`57`	`67`	`self = [super init];`
`58`	`68`	`_info = info;`
`59`		`- // Don't expose objects with an invalid type.`
	`69`	`+ // Don't expose objects with an out-of-range tag or invalid type`
	`70`	`+ VerifyOrReturnValue(chip::SetupPayload::IsVendorTag(_info.tag), nil);`
`60`	`71`	`VerifyOrReturnValue(self.type != MTROptionalQRCodeInfoTypeUnknown, nil);`
`61`	`72`	`return self;`
`62`	`73`	`}`
`@@ -97,9 +108,9 @@ - (MTROptionalQRCodeInfoType)type`
`97`	`108`	`return MTROptionalQRCodeInfoTypeUnknown;`
`98`	`109`	`}`
`99`	`110`
`100`		`-- (uint8_t)tagNumber`
	`111`	`+- (NSNumber *)tag`
`101`	`112`	`{`
`102`		`- return _info.tag;`
	`113`	`+ return @(_info.tag);`
`103`	`114`	`}`
`104`	`115`
`105`	`116`	`- (NSNumber *)integerValue`
`@@ -142,7 +153,7 @@ - (BOOL)isEqual:(id)object`
`142`	`153`
`143`	`154`	`- (NSString *)description`
`144`	`155`	`{`
`145`		`- return [NSString stringWithFormat:@"0x%02x=%@", self.tagNumber, self.integerValue ?: self.stringValue];`
	`156`	`+ return [NSString stringWithFormat:@"0x%02x=%@", self.tag.unsignedCharValue, self.integerValue ?: self.stringValue];`
`146`	`157`	`}`
`147`	`158`
`148`	`159`	`@end`
`@@ -431,21 +442,21 @@ - (void)setSerialNumber:(nullable NSString *)serialNumber`
`431`	`442`	`return infos;`
`432`	`443`	`}`
`433`	`444`
`434`		`-- (MTROptionalQRCodeInfo *)vendorElementWithTag:(uint8_t)tag`
	`445`	`+- (MTROptionalQRCodeInfo )vendorElementWithTag:(NSNumber )tag`
`435`	`446`	`{`
`436`	`447`	`chip::OptionalQRCodeInfo element;`
`437`		`- VerifyOrReturnValue(_payload.getOptionalVendorData(tag, element) == CHIP_NO_ERROR, nil);`
	`448`	`+ VerifyOrReturnValue(_payload.getOptionalVendorData(ValidateVendorTag(tag), element) == CHIP_NO_ERROR, nil);`
`438`	`449`	`return [[MTROptionalQRCodeInfo alloc] initWithQRCodeInfo:element];`
`439`	`450`	`}`
`440`	`451`
`441`		`-- (void)removeVendorElementWithTag:(uint8_t)tag`
	`452`	`+- (void)removeVendorElementWithTag:(NSNumber *)tag`
`442`	`453`	`{`
`443`		`- _payload.removeOptionalVendorData(tag);`
	`454`	`+ _payload.removeOptionalVendorData(ValidateVendorTag(tag));`
`444`	`455`	`}`
`445`	`456`
`446`	`457`	`- (void)addOrReplaceVendorElement:(MTROptionalQRCodeInfo *)element`
`447`	`458`	`{`
`448`		`- MTRVerifyArgumentOrDie(chip::SetupPayload::IsVendorTag(element.tagNumber), @"element.tagNumber");`
	`459`	`+ MTRVerifyArgumentOrDie(element != nil, @"element");`
`449`	`460`	`CHIP_ERROR err = [element addAsVendorElementTo:_payload];`
`450`	`461`	`VerifyOrDieWithMsg(err == CHIP_NO_ERROR, NotSpecified, "Internal error: %" CHIP_ERROR_FORMAT, err.Format());`
`451`	`462`	`}`
`@@ -615,19 +626,14 @@ @implementation MTROptionalQRCodeInfo (Deprecated)`
`615`	`626`
`616`	`627`	`- (instancetype)init`
`617`	`628`	`{`
`618`		`- return [self initWithTag:0xff stringValue:@""];`
	`629`	`+ return [self initWithTag:@0xff stringValue:@""];`
`619`	`630`	`}`
`620`	`631`
`621`	`632`	`- (void)setType:(MTROptionalQRCodeInfoType)type`
`622`	`633`	`{`
`623`	`634`	`/* ignored */`
`624`	`635`	`}`
`625`	`636`
`626`		`-- (NSNumber *)tag`
`627`		`-{`
`628`		`- return @(self.tagNumber);`
`629`		`-}`
`630`		`-`
`631`	`637`	`- (void)setTag:(NSNumber *)tag`
`632`	`638`	`{`
`633`	`639`	`/* ignored */`