raise MTRDevice for subclassing
#34757
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with SemanticDiff. |
woody-apple
reviewed
Aug 2, 2024
|
PR #34757: Size comparison from 26c816a to 0977039 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
kiel-apple
force-pushed
the
darwin-raise-mtrdevice
branch
from
0977039 to
2bd9d0b
Compare
woody-apple
reviewed
Aug 2, 2024
woody-apple
reviewed
Aug 2, 2024
woody-apple
reviewed
Aug 2, 2024
woody-apple
reviewed
Aug 2, 2024
woody-apple
reviewed
Aug 2, 2024
| @@ -366,7 +384,7 @@ - (BOOL)isEqual:(id)object | |||
| // better behavior. | |||
| #define MTRDEVICE_SUBSCRIPTION_LATENCY_NEW_VALUE_WEIGHT (1.0 / 3.0) | |||
|
|
|||
| @interface MTRDevice () | |||
| @interface MTRDevice_Concrete () | |||
There was a problem hiding this comment.
We should duplicate the file for step 1, not rename.
That sets us up nicely for the next stage where we take all of the methods in MTRDevice and put warnings in them or logs, when they're called on the abstract version of the class
There was a problem hiding this comment.
Redid the first commit to duplicate the .mm file.
|
PR #34757: Size comparison from 1e9082f to a075fb5 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| @class MTRDeviceController; | ||
|
|
||
| MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1)) | ||
| @interface MTRDevice_Concrete : MTRDevice |
There was a problem hiding this comment.
"Concrete" doesn't really mean anything. Presumably other implementations of MTRDevice will also be "concrete"...
What's special about this impl? Really, the fact that it does things with the Matter SDK directly, right? So maybe MTRDeviceInProcess? MTRDeviceDirect? Having a hard time with the naming....
There was a problem hiding this comment.
Direct crossed my mind.
There was a problem hiding this comment.
Given this is a hidden class, we can rename it at any time, let's get this merged post tests passing, and please remove this availability flags here, since it's not public API
kiel-apple
force-pushed
the
darwin-raise-mtrdevice
branch
from
a075fb5 to
dea4e7d
Compare
|
PR #34757: Size comparison from da6dd90 to dea4e7d Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #34757: Size comparison from da6dd90 to 6cbef72 Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
start closing off `MTRDevice` direct use add a note to self / reviewers fix `newBaseDevice` / `removeExpectedValue[s]` error ty @bzbarsky-apple move `MTRDevice_Concrete.h` to Project scope was Public note to self about coming change in MTRDeviceController move some MTRDevice utilities to MTRDevice_Internal.h where they are at least shared between MTRDevice and MTRDevice_Concrete. but probably they merit their own files - the header is getting heavy add subclass-facing init to `MTRDevice` superclass for `MTRDevice_Concrete` code was `NSObject`, but now is `MTRDevice`, which hides its `init`s. fix build of `MTRDevice_Internal.h` Revert "move some MTRDevice utilities" This reverts commit ba7331f. fix MTRDevice_Concrete block-scoped pointer types move clamped number to utilities remove duplicated MTRClampedNumber implementations more `MTRClampedNumber` cleanup duplicate MTRDeviceDelegateInfo for now restore prematurely removed `MTRDevice` methods move common `MTRDeviceClusterData` keys remove now-obsolete include for CodeUtils remove duplicate `MTRDeviceClusterData` remove duplicate key symbols from `MTRDevice_Concrete` remove availability annotations for nonpublic API Restyled by whitespace Restyled by clang-format remove superfluous init/new signatures available by default
kiel-apple
force-pushed
the
darwin-raise-mtrdevice
branch
from
168bb3a to
d774007
Compare
|
PR #34757: Size comparison from 83dc1c8 to d774007 Full report (84 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MTRDevicethat exists toMTRDeviceConcrete(open to other names)MTRSwiftDeviceTestsMTRDeviceConcrete's API, but with "this method is not implemented in the base class" stubs for all methodsMTRDeviceConcreteinstances that should instead beMTRDevice-generic