Skip to content
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

Make it possible to call Encode/Decode on a subclass of a cluster-object struct. #11014

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

Our enable_if machinery for Encode/Decode only worked for structs of
type T if there was a T::Encode method. That made it impossible to
subclass a struct (e.g. with the subclass providing backing storage
for the struct's Spans) and call Encode on the subclass instance
without jumping through some sort of testing hoops.

This change changes our test from "T::Encode exists with the right
signature" to "an instance of T has a .Encode that when called with
the right arguments returns the right return type", which lets
subclassing work as desired.

Problem

See above.

Change overview

See above.

Testing

Tree compiles.

…ect struct.

Our enable_if machinery for Encode/Decode only worked for structs of
type T if there was a T::Encode method.  That made it impossible to
subclass a struct (e.g. with the subclass providing backing storage
for the struct's Spans) and call Encode on the subclass instance
without jumping through some sort of testing hoops.

This change changes our test from "T::Encode exists with the right
signature" to "an instance of T has a .Encode that when called with
the right arguments returns the right return type", which lets
subclassing work as desired.
@github-actions
Copy link

github-actions bot commented Oct 27, 2021

PR #11014: Size comparison from 4b1bc90 to 25b1796

Increases (1 build for esp32)
platform target config section 4b1bc90 25b1796 change % change
esp32 all-clusters-app m5stack (read only) 906171 906259 88 0.0
.flash.text 906171 906259 88 0.0
Full report (36 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section 4b1bc90 25b1796 change % change
efr32 lighting-app BRD4161A (read only) 736512 736512 0 0.0
(read/write) 115444 115444 0 0.0
.bss 113692 113692 0 0.0
.data 1752 1752 0 0.0
.text 736504 736504 0 0.0
BRD4161A+rpc (read only) 723936 723936 0 0.0
(read/write) 132052 132052 0 0.0
.bss 130196 130196 0 0.0
.data 1852 1852 0 0.0
.text 723928 723928 0 0.0
lock-app BRD4161A (read only) 715792 715792 0 0.0
(read/write) 113260 113260 0 0.0
.bss 111548 111548 0 0.0
.data 1712 1712 0 0.0
.text 715784 715784 0 0.0
window-app BRD4161A (read only) 716676 716676 0 0.0
(read/write) 113576 113576 0 0.0
.bss 111860 111860 0 0.0
.data 1716 1716 0 0.0
.text 716668 716668 0 0.0
esp32 all-clusters-app c3devkit (read only) 875074 875074 0 0.0
(read/write) 1306128 1306128 0 0.0
.dram0.bss 58240 58240 0 0.0
.dram0.data 16464 16464 0 0.0
.flash.rodata 198192 198192 0 0.0
.flash.text 875074 875074 0 0.0
.iram0.text 57564 57564 0 0.0
m5stack (read only) 906171 906259 88 0.0
(read/write) 425936 425936 0 0.0
.dram0.bss 60744 60744 0 0.0
.dram0.data 32084 32084 0 0.0
.flash.rodata 206944 206944 0 0.0
.flash.text 906171 906259 88 0.0
.iram0.text 125115 125115 0 0.0
k32w lighting-app k32w061+se05x+release (read/write) 700600 700600 0 0.0
.bss 78720 78720 0 0.0
.data 1900 1900 0 0.0
.text 614180 614180 0 0.0
lock-app k32w061+debug (read/write) 592404 592404 0 0.0
.bss 69196 69196 0 0.0
.data 1864 1864 0 0.0
.text 515544 515544 0 0.0
shell k32w061+debug (read/write) 425204 425204 0 0.0
.bss 63256 63256 0 0.0
.data 672 672 0 0.0
.text 359572 359572 0 0.0
linux all-clusters-app debug (read only) 1626393 1626393 0 0.0
(read/write) 117344 117344 0 0.0
.bss 50288 50288 0 0.0
.data 978 978 0 0.0
.data.rel.ro 60832 60832 0 0.0
.dynamic 592 592 0 0.0
.got 4088 4088 0 0.0
.init 27 27 0 0.0
.init_array 520 520 0 0.0
.rodata 134677 134677 0 0.0
.text 1360002 1360002 0 0.0
bridge-app debug+rpc (read only) 1271485 1271485 0 0.0
(read/write) 85096 85096 0 0.0
.bss 51856 51856 0 0.0
.data 976 976 0 0.0
.data.rel.ro 27272 27272 0 0.0
.dynamic 592 592 0 0.0
.got 3952 3952 0 0.0
.init 27 27 0 0.0
.init_array 400 400 0 0.0
.rodata 109884 109884 0 0.0
.text 1066261 1066261 0 0.0
chip-tool debug (read only) 4117941 4117941 0 0.0
(read/write) 119448 119448 0 0.0
.bss 17680 17680 0 0.0
.data 1584 1584 0 0.0
.data.rel.ro 94784 94784 0 0.0
.dynamic 592 592 0 0.0
.got 4368 4368 0 0.0
.init 27 27 0 0.0
.init_array 416 416 0 0.0
.rodata 208208 208208 0 0.0
.text 3653125 3653125 0 0.0
lighting-app debug+rpc (read only) 1523369 1523369 0 0.0
(read/write) 101520 101520 0 0.0
.bss 41176 41176 0 0.0
.data 1106 1106 0 0.0
.data.rel.ro 53968 53968 0 0.0
.dynamic 608 608 0 0.0
.got 4112 4112 0 0.0
.init 27 27 0 0.0
.init_array 528 528 0 0.0
.rodata 127185 127185 0 0.0
.text 1264162 1264162 0 0.0
ota-provider-app debug (read only) 1232161 1232161 0 0.0
(read/write) 67776 67776 0 0.0
.bss 37440 37440 0 0.0
.data 752 752 0 0.0
.data.rel.ro 24488 24488 0 0.0
.dynamic 592 592 0 0.0
.got 4016 4016 0 0.0
.init 27 27 0 0.0
.init_array 440 440 0 0.0
.rodata 110504 110504 0 0.0
.text 1025698 1025698 0 0.0
ota-requestor-app debug (read only) 1289297 1289297 0 0.0
(read/write) 76264 76264 0 0.0
.bss 44960 44960 0 0.0
.data 816 816 0 0.0
.data.rel.ro 25352 25352 0 0.0
.dynamic 592 592 0 0.0
.got 3984 3984 0 0.0
.init 27 27 0 0.0
.init_array 512 512 0 0.0
.rodata 127392 127392 0 0.0
.text 1064306 1064306 0 0.0
shell debug (read only) 777097 777097 0 0.0
(read/write) 57376 57376 0 0.0
.bss 16136 16136 0 0.0
.data 242 242 0 0.0
.data.rel.ro 36496 36496 0 0.0
.dynamic 592 592 0 0.0
.got 3528 3528 0 0.0
.init 27 27 0 0.0
.init_array 336 336 0 0.0
.rodata 76527 76527 0 0.0
.text 599442 599442 0 0.0
tv-app debug (read only) 1750105 1750105 0 0.0
(read/write) 281432 281432 0 0.0
.bss 215568 215568 0 0.0
.data 2032 2032 0 0.0
.data.rel.ro 58192 58192 0 0.0
.dynamic 592 592 0 0.0
.got 4408 4408 0 0.0
.init 27 27 0 0.0
.init_array 608 608 0 0.0
.rodata 152184 152184 0 0.0
.text 1456210 1456210 0 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2289384 2289384 0 0.0
.bss 179604 179604 0 0.0
.data 5192 5192 0 0.0
.heap 851648 851648 0 0.0
.text 1251984 1251984 0 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2257072 2257072 0 0.0
.bss 171060 171060 0 0.0
.data 5464 5464 0 0.0
.heap 859920 859920 0 0.0
.text 1219672 1219672 0 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2235104 2235104 0 0.0
.bss 169988 169988 0 0.0
.data 5432 5432 0 0.0
.heap 861024 861024 0 0.0
.text 1197704 1197704 0 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1139680 1139680 0 0.0
.bss 11760 11760 0 0.0
.data 4360 4360 0 0.0
.heap 1020328 1020328 0 0.0
.text 103064 103064 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 862471 862471 0 0.0
bss 112136 112136 0 0.0
rodata 97200 97200 0 0.0
text 577512 577512 0 0.0
nrf52840dk_nrf52840+rpc (read/write) 823311 823311 0 0.0
bss 108380 108380 0 0.0
rodata 87976 87976 0 0.0
text 550704 550704 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 787510 787510 0 0.0
bss 113512 113512 0 0.0
rodata 92440 92440 0 0.0
text 506984 506984 0 0.0
lock-app nrf52840dk_nrf52840 (read/write) 839415 839415 0 0.0
bss 111208 111208 0 0.0
rodata 93600 93600 0 0.0
text 559072 559072 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 764458 764458 0 0.0
bss 112584 112584 0 0.0
rodata 88860 88860 0 0.0
text 488536 488536 0 0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 497339 497339 0 0.0
bss 51824 51824 0 0.0
rodata 45776 45776 0 0.0
text 339456 339456 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 843815 843815 0 0.0
bss 111308 111308 0 0.0
rodata 94800 94800 0 0.0
text 562176 562176 0 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 839135 839135 0 0.0
bss 111208 111208 0 0.0
rodata 93576 93576 0 0.0
text 558816 558816 0 0.0
shell nrf52840dk_nrf52840 (read/write) 776531 776531 0 0.0
bss 109072 109072 0 0.0
rodata 72536 72536 0 0.0
text 520324 520324 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 691586 691586 0 0.0
bss 110056 110056 0 0.0
rodata 67180 67180 0 0.0
text 440936 440936 0 0.0
p6 lock-app default (read/write) 2168520 2168520 0 0.0
.bss 67184 67184 0 0.0
.data 2416 2416 0 0.0
.heap 963744 963744 0 0.0
.text 1126784 1126784 0 0.0
qpg lighting-app qpg6100+debug (read only) 490760 490760 0 0.0
(read/write) 114144 114144 0 0.0
.bss 52416 52416 0 0.0
.data 1000 1000 0 0.0
.text 485440 485440 0 0.0
lock-app qpg6100+debug (read only) 466980 466980 0 0.0
(read/write) 114140 114140 0 0.0
.bss 51368 51368 0 0.0
.data 956 956 0 0.0
.text 461660 461660 0 0.0
persistent-storage-app qpg6100+debug (read only) 155220 155220 0 0.0
(read/write) 114140 114140 0 0.0
.bss 27752 27752 0 0.0
.data 372 372 0 0.0
.text 149900 149900 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 664066 664066 0 0.0
bss 69944 69944 0 0.0
noinit 33216 33216 0 0.0
text 457976 457976 0 0.0

@bzbarsky-apple bzbarsky-apple merged commit 92d530a into project-chip:master Oct 27, 2021
@bzbarsky-apple bzbarsky-apple deleted the more-generic-struct-encode branch October 27, 2021 19:57
JasonLiuZhuoCheng pushed a commit to JasonLiuZhuoCheng/connectedhomeip that referenced this pull request Oct 28, 2021
…ect struct. (project-chip#11014)

Our enable_if machinery for Encode/Decode only worked for structs of
type T if there was a T::Encode method.  That made it impossible to
subclass a struct (e.g. with the subclass providing backing storage
for the struct's Spans) and call Encode on the subclass instance
without jumping through some sort of testing hoops.

This change changes our test from "T::Encode exists with the right
signature" to "an instance of T has a .Encode that when called with
the right arguments returns the right return type", which lets
subclassing work as desired.
carol-apple pushed a commit to carol-apple/connectedhomeip that referenced this pull request Oct 28, 2021
…ect struct. (project-chip#11014)

Our enable_if machinery for Encode/Decode only worked for structs of
type T if there was a T::Encode method.  That made it impossible to
subclass a struct (e.g. with the subclass providing backing storage
for the struct's Spans) and call Encode on the subclass instance
without jumping through some sort of testing hoops.

This change changes our test from "T::Encode exists with the right
signature" to "an instance of T has a .Encode that when called with
the right arguments returns the right return type", which lets
subclassing work as desired.
PSONALl pushed a commit to PSONALl/connectedhomeip that referenced this pull request Dec 3, 2021
…ect struct. (project-chip#11014)

Our enable_if machinery for Encode/Decode only worked for structs of
type T if there was a T::Encode method.  That made it impossible to
subclass a struct (e.g. with the subclass providing backing storage
for the struct's Spans) and call Encode on the subclass instance
without jumping through some sort of testing hoops.

This change changes our test from "T::Encode exists with the right
signature" to "an instance of T has a .Encode that when called with
the right arguments returns the right return type", which lets
subclassing work as desired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants