-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes: [OPSTATE] specific phase in the list should not null #31176
Fixes: [OPSTATE] specific phase in the list should not null #31176
Conversation
PR #31176: Size comparison from 061ff86 to 407a397 Decreases (2 builds for efr32, linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
PR #31176: Size comparison from 827f6ca to b4b7273 Increases (1 build for linux)
Decreases (2 builds for efr32, linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND, | ||
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND, | |
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null. | |
* | |
* If CHIP_ERROR_NOT_FOUND is returned for index 0, that indicates that the PhaseList attribute is null | |
* (there are no phases defined at all). | |
* |
and the same in all the other places where this got copy/pasted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably more of a spec question than an SDK question, but why have a nullable list to begin with? Per 17.18.1
Composite data types that have a length (i.e. octet string and list), and derived types that have those as the base type, SHALL NOT differentiate semantically between the null value and the empty (zero length) value.
This seems like extra wrappers for no good reason. Or is this just for backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec could have gone either way here.... There are various opinions about which is better (e.g. null is one less byte on the wire than an empty list is). But for purposes of the SDK, this is what the spec says, and yes, this has already shipped as nullable, so at this point we can't really change the spec.
PR #31176: Size comparison from 9279628 to ac0f34e Increases (1 build for linux)
Decreases (2 builds for efr32, linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
PR #31176: Size comparison from 8686f21 to 7b93188 Increases (1 build for linux)
Decreases (2 builds for efr32, linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
Fixes #27931