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

Fix comparison of a nullable value to a non-nullable saved value. #21507

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

The code generated for the case when a non-nullable value was saved via saveAs
and then a nullable value was being compared to it did not compile, because we
would try to do .IsNull() and .Value() on the non-nullable saved value.

The changes here check whether the saved value is actually nullable and generate
code accordingly, instead of assuming that if we're comparing a nullable to
something the something must also be nullable.

Problem

See above. This came up in #21361

Change overview

See above.

Testing

Verified that regenerating #21361 with this change makes it generate correct code.

The code generated for the case when a non-nullable value was saved via saveAs
and then a nullable value was being compared to it did not compile, because we
would try to do .IsNull() and .Value() on the non-nullable saved value.

The changes here check whether the saved value is actually nullable and generate
code accordingly, instead of assuming that if we're comparing a nullable to
something the something must also be nullable.
@github-actions
Copy link

github-actions bot commented Aug 2, 2022

PR #21507: Size comparison from 7a4f84f to cc47116

Increases (3 builds for cc13x2_26x2, nrfconnect)
platform target config section 7a4f84f cc47116 change % change
cc13x2_26x2 lock-mtd LP_CC2652R7 (read only) 655419 655427 8 0.0
.text 553408 553416 8 0.0
shell LP_CC2652R7 (read/write) 184296 184304 8 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 text 811796 811800 4 0.0
Decreases (4 builds for cc13x2_26x2, telink)
platform target config section 7a4f84f cc47116 change % change
cc13x2_26x2 lock-mtd LP_CC2652R7 (read/write) 181836 181828 -8 -0.0
shell LP_CC2652R7 (read only) 662606 662598 -8 -0.0
.text 576844 576836 -8 -0.0
telink light-switch-app tlsr9518adk80d text 569296 569294 -2 -0.0
lighting-app tlsr9518adk80d text 586184 586182 -2 -0.0
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section 7a4f84f cc47116 change % change
bl602 lighting-app bl602 (read/write) 1382530 1382530 0 0.0
.bss 117626 117626 0 0.0
.data 4480 4480 0 0.0
.text 1052640 1052640 0 0.0
bl602+rpc (read/write) 1427930 1427930 0 0.0
.bss 125066 125066 0 0.0
.data 4600 4600 0 0.0
.text 1084304 1084304 0 0.0
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 669899 669899 0 0.0
(read/write) 181484 181484 0 0.0
.bss 74276 74276 0 0.0
.data 3372 3372 0 0.0
.rodata 88467 88467 0 0.0
.text 581116 581116 0 0.0
all-clusters-minimal-app LP_CC2652R7 (read only) 635403 635403 0 0.0
(read/write) 157844 157844 0 0.0
.bss 73556 73556 0 0.0
.data 3372 3372 0 0.0
.rodata 77691 77691 0 0.0
.text 557388 557388 0 0.0
lock-ftd LP_CC2652R7 (read only) 672871 672871 0 0.0
(read/write) 168696 168696 0 0.0
.bss 71348 71348 0 0.0
.data 3296 3296 0 0.0
.rodata 76535 76535 0 0.0
.text 595856 595856 0 0.0
lock-mtd LP_CC2652R7 (read only) 655419 655427 8 0.0
(read/write) 181836 181828 -8 -0.0
.bss 67036 67036 0 0.0
.data 3296 3296 0 0.0
.rodata 101531 101531 0 0.0
.text 553408 553416 8 0.0
pump-app LP_CC2652R7 (read only) 681367 681367 0 0.0
(read/write) 161008 161008 0 0.0
.bss 71388 71388 0 0.0
.data 3296 3296 0 0.0
.rodata 88919 88919 0 0.0
.text 591964 591964 0 0.0
pump-controller-app LP_CC2652R7 (read only) 666943 666943 0 0.0
(read/write) 175568 175568 0 0.0
.bss 71524 71524 0 0.0
.data 3292 3292 0 0.0
.rodata 84743 84743 0 0.0
.text 581720 581720 0 0.0
shell LP_CC2652R7 (read only) 662606 662598 -8 -0.0
(read/write) 184296 184304 8 0.0
.bss 76596 76596 0 0.0
.data 3376 3376 0 0.0
.rodata 85446 85446 0 0.0
.text 576844 576836 -8 -0.0
cyw30739 light cyw930739m2evb_01 (read/write) 584054 584054 0 0.0
.app_xip_area 460864 460864 0 0.0
.bss 65632 65632 0 0.0
.data 744 744 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
lock cyw930739m2evb_01 (read/write) 590318 590318 0 0.0
.app_xip_area 462376 462376 0 0.0
.bss 70384 70384 0 0.0
.data 748 748 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 589762 589762 0 0.0
.app_xip_area 467380 467380 0 0.0
.bss 64880 64880 0 0.0
.data 688 688 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read/write) 1089040 1089040 0 0.0
.bss 133268 133268 0 0.0
.data 2064 2064 0 0.0
.text 953688 953688 0 0.0
BRD4161A+rpc (read/write) 1143332 1143332 0 0.0
.bss 149948 149948 0 0.0
.data 2276 2276 0 0.0
.text 991088 991088 0 0.0
BRD4161A+rs911x (read/write) 975008 975008 0 0.0
.bss 161744 161744 0 0.0
.data 2048 2048 0 0.0
.text 811196 811196 0 0.0
lock-app BRD4161A+wf200 (read/write) 1130472 1130472 0 0.0
.bss 144400 144400 0 0.0
.data 2056 2056 0 0.0
.text 983996 983996 0 0.0
window-app BRD4161A (read/write) 1082332 1082332 0 0.0
.bss 134748 134748 0 0.0
.data 2092 2092 0 0.0
.text 945468 945468 0 0.0
esp32 all-clusters-app c3devkit (read only) 1024312 1024312 0 0.0
(read/write) 1487042 1487042 0 0.0
.dram0.bss 70336 70336 0 0.0
.dram0.data 14600 14600 0 0.0
.flash.rodata 216672 216672 0 0.0
.flash.text 1024312 1024312 0 0.0
.iram0.text 62902 62902 0 0.0
m5stack (read only) 1077807 1077807 0 0.0
(read/write) 489064 489064 0 0.0
.dram0.bss 75840 75840 0 0.0
.dram0.data 34144 34144 0 0.0
.flash.rodata 247084 247084 0 0.0
.flash.text 1072423 1072423 0 0.0
.iram0.text 123267 123267 0 0.0
k32w light k32w0+release (read/write) 643448 643448 0 0.0
.bss 69720 69720 0 0.0
.data 2044 2044 0 0.0
.text 568956 568956 0 0.0
lock k32w0+release (read/write) 701816 701816 0 0.0
.bss 70184 70184 0 0.0
.data 2052 2052 0 0.0
.text 626852 626852 0 0.0
linux chip-tool-ipv6only arm64 (read only) 9939852 9939852 0 0.0
(read/write) 690049 690049 0 0.0
.bss 32897 32897 0 0.0
.data 3272 3272 0 0.0
.data.rel.ro 635336 635336 0 0.0
.dynamic 560 560 0 0.0
.got 13592 13592 0 0.0
.init 24 24 0 0.0
.init_array 192 192 0 0.0
.rodata 470892 470892 0 0.0
.text 7851476 7851476 0 0.0
thermostat-no-ble arm64 (read only) 2349996 2349996 0 0.0
(read/write) 141761 141761 0 0.0
.bss 55329 55329 0 0.0
.data 1672 1672 0 0.0
.data.rel.ro 75976 75976 0 0.0
.dynamic 560 560 0 0.0
.got 5016 5016 0 0.0
.init 24 24 0 0.0
.init_array 408 408 0 0.0
.rodata 139316 139316 0 0.0
.text 1973168 1973168 0 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2453288 2453288 0 0.0
.bss 214548 214548 0 0.0
.data 5872 5872 0 0.0
.text 1415932 1415932 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1176491 1176491 0 0.0
bss 143224 143224 0 0.0
rodata 142620 142620 0 0.0
text 811796 811800 4 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 (read/write) 1156379 1156379 0 0.0
bss 142460 142460 0 0.0
rodata 134148 134148 0 0.0
text 800948 800948 0 0.0
p6 all-clusters-app default (read only) 881528 881528 0 0.0
(read/write) 1689156 1689156 0 0.0
.bss 149168 149168 0 0.0
.data 2648 2648 0 0.0
.text 1528952 1528952 0 0.0
all-clusters-minimal-app default (read only) 882248 882248 0 0.0
(read/write) 1633188 1633188 0 0.0
.bss 148448 148448 0 0.0
.data 2648 2648 0 0.0
.text 1473704 1473704 0 0.0
light-app default (read only) 890568 890568 0 0.0
(read/write) 1553444 1553444 0 0.0
.bss 140336 140336 0 0.0
.data 2440 2440 0 0.0
.text 1402280 1402280 0 0.0
lock-app default (read only) 886072 886072 0 0.0
(read/write) 1592100 1592100 0 0.0
.bss 144816 144816 0 0.0
.data 2456 2456 0 0.0
.text 1436440 1436440 0 0.0
telink light-switch-app tlsr9518adk80d (read/write) 805440 805440 0 0.0
bss 70976 70976 0 0.0
noinit 43488 43488 0 0.0
text 569296 569294 -2 -0.0
lighting-app tlsr9518adk80d (read/write) 825880 825880 0 0.0
bss 71820 71820 0 0.0
noinit 43488 43488 0 0.0
text 586184 586182 -2 -0.0

@woody-apple woody-apple added the sve label Aug 2, 2022
@woody-apple woody-apple enabled auto-merge (squash) August 2, 2022 00:45
@woody-apple woody-apple merged commit 97134c5 into project-chip:master Aug 2, 2022
@bzbarsky-apple bzbarsky-apple deleted the fix-nullable-vs-non-saveAs branch August 2, 2022 02:20
github-actions bot pushed a commit that referenced this pull request Aug 2, 2022
…1507)

The code generated for the case when a non-nullable value was saved via saveAs
and then a nullable value was being compared to it did not compile, because we
would try to do .IsNull() and .Value() on the non-nullable saved value.

The changes here check whether the saved value is actually nullable and generate
code accordingly, instead of assuming that if we're comparing a nullable to
something the something must also be nullable.
woody-apple added a commit that referenced this pull request Aug 2, 2022
…1507) (#21516)

The code generated for the case when a non-nullable value was saved via saveAs
and then a nullable value was being compared to it did not compile, because we
would try to do .IsNull() and .Value() on the non-nullable saved value.

The changes here check whether the saved value is actually nullable and generate
code accordingly, instead of assuming that if we're comparing a nullable to
something the something must also be nullable.

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
isiu-apple pushed a commit to isiu-apple/connectedhomeip that referenced this pull request Sep 16, 2022
…oject-chip#21507)

The code generated for the case when a non-nullable value was saved via saveAs
and then a nullable value was being compared to it did not compile, because we
would try to do .IsNull() and .Value() on the non-nullable saved value.

The changes here check whether the saved value is actually nullable and generate
code accordingly, instead of assuming that if we're comparing a nullable to
something the something must also be nullable.
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.

None yet

2 participants