RFC: Remove the ReadClient* parameter to ReadClient::Callbacks #14013
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
|
PR #14013: Size comparison from f0f5de0 to a21b332 Decreases (9 builds for cyw30739, k32w, p6, qpg, telink)
Full report (10 builds for cyw30739, k32w, p6, qpg, telink)
|
mspang
force-pushed
the
for-chip/readclient-arg
branch
from
a21b332 to
feb7df8
Compare
|
PR #14013: Size comparison from e169fcf to feb7df8 Decreases (24 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (33 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
mspang
force-pushed
the
for-chip/readclient-arg
branch
from
feb7df8 to
2e3f1ea
Compare
mspang
force-pushed
the
for-chip/readclient-arg
branch
from
2e3f1ea to
72c9d7e
Compare
|
PR #14013: Size comparison from 0902641 to 72c9d7e Decreases (23 builds for cyw30739, efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (31 builds for cyw30739, efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
mspang
force-pushed
the
for-chip/readclient-arg
branch
from
72c9d7e to
4af2028
Compare
|
PR #14013: Size comparison from 2e0f64d to 4af2028 Decreases (18 builds for cyw30739, efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
Full report (22 builds for cyw30739, efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex and
hawk248
pullapprove
bot
requested review from
selissia,
tcarmelveilleux,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
bzbarsky-apple
approved these changes
Jan 26, 2022
andy31415
approved these changes
Jan 26, 2022
|
fast track: generally a refector except the deletion bits which was approved by a domain owner. PR has been up sufficient time for cross timezone review. |
mspang
force-pushed
the
for-chip/readclient-arg
branch
2 times, most recently
from
c4ecfb6 to
ee34073
Compare
Currently each ReadClient callback takes as a parameter a pointer to the ReadClient itself. This is not very useful and introduces undesirable coupling: It prevents creating another class (say, MockReadClient or NextGenReadClient) that is compatible with existing callbacks without the same concrete implementation class. This is showing up very concretely in tests that are passing "nullptr" since their test double cannot be passed through the callback.
mspang
force-pushed
the
for-chip/readclient-arg
branch
from
ee34073 to
c7aa9a1
Compare
mrjerryjohns
approved these changes
Jan 26, 2022
|
PR #14013: Size comparison from 5429dca to c7aa9a1 Decreases (15 builds for cyw30739, efr32, esp32, k32w, linux, p6, qpg, telink)
Full report (17 builds for cyw30739, efr32, esp32, k32w, linux, p6, qpg, telink)
|
|
@mspang Why did you revert the mbed-os-posix-socket version here? |
selissia
pushed a commit
to selissia/connectedhomeip
that referenced
this pull request
Jan 28, 2022
…ct-chip#14013) Currently each ReadClient callback takes as a parameter a pointer to the ReadClient itself. This is not very useful and introduces undesirable coupling: It prevents creating another class (say, MockReadClient or NextGenReadClient) that is compatible with existing callbacks without the same concrete implementation class. This is showing up very concretely in tests that are passing "nullptr" since their test double cannot be passed through the callback.
I would guess accidental due to bad merge.... |
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Feb 1, 2022
The changes in project-chip#14013 probably did not mean to roll back the submodule update from project-chip#12616.
|
I created #14613 to fix that. |
andy31415
pushed a commit
that referenced
this pull request
Feb 1, 2022
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.
Problem
Currently each ReadClient callback takes as a parameter a pointer to the
ReadClient itself. This is not very useful and introduces undesirable
coupling: It prevents creating another class (say, MockReadClient or
NextGenReadClient) that is compatible with existing callbacks
without the same concrete implementation class.
This is showing up very concretely in tests that are passing "nullptr"
since their test double cannot be passed through the callback.
Change overview
Remove the ReadClient pointer from each callback. There are few
uses of this, and none are crucial, as the creator of the read client
already has as pointer to it.
The only nontrivial use of of this is freeing read clients in OnDone,
which is quite subtle and problematic because shutting down
with pending IM callbacks will leak the client. In any case, we
can preserve the semantics by simply adopting the client
into the callback.
Testing
Unit tests.