[Bugfix] remove Instance friend class and move setInstance to pulbic section #41204
+26
−7
Conversation
This file contains hidden or 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
There was a problem hiding this comment.
Code Review
This pull request removes the friend class relationship between Instance and Delegate by making Delegate::SetInstance public. This is a good step for decoupling. The change also correctly introduces const and non-const overloads for GetInstance, improving const-correctness. My main feedback is regarding the now-public SetInstance method, which could be misused. I've added a comment with a suggestion to make it safer.
src/app/clusters/operational-state-server/operational-state-server.h
Outdated
Show resolved
Hide resolved
|
PR #41204: Size comparison from 60d7fa3 to d2a74c2 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
|
/review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the design of the OperationalState::Delegate class by removing the friend class Instance declaration and fixing a const-correctness issue in GetInstance. Moving SetInstance to the public section is a necessary change to eliminate the friend dependency, and the addition of an assert in SetInstance is a good defensive measure. I have one suggestion to improve the documentation for the now-public SetInstance method to clarify its intended use and prevent misuse.
|
PR #41204: Size comparison from 60d7fa3 to e07a888 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
|
PR #41204: Size comparison from 60d7fa3 to e72c67a Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
|
PR #41204: Size comparison from 60d7fa3 to 88154ec Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41204 +/- ##
=======================================
Coverage 50.94% 50.94%
=======================================
Files 1378 1378
Lines 100698 100703 +5
Branches 13058 13058
=======================================
+ Hits 51303 51307 +4
- Misses 49395 49396 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Delegate class to remove a friend declaration, making SetInstance public with added runtime checks. It also corrects a const-correctness violation in GetInstance by providing proper overloads. While these changes improve code quality and safety, the modification to GetInstance introduces a breaking API change that should be communicated to users. There is also a minor typo ('pulbic') in the pull request title.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes an unnecessary friend class declaration and moves the SetInstance method to the public section of the Delegate class. It also correctly fixes a const-correctness issue in the GetInstance method by providing proper const and non-const overloads. The changes improve code quality and safety. I've identified one potential issue regarding a dangling pointer in the Delegate if the Instance is destroyed first, which could lead to a use-after-free. I have provided a suggestion to address this by updating the documentation and recommending a change in the Instance destructor.
src/app/clusters/operational-state-server/operational-state-server.h
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully removes the friend class declaration in Delegate by making SetInstance public, which improves encapsulation. The changes also correctly fix a const-correctness issue with GetInstance and improve object lifecycle management by clearing the delegate's instance pointer in the Instance destructor. I have one suggestion to improve the accuracy of a comment.
src/app/clusters/operational-state-server/operational-state-server.h
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request provides a good set of improvements to the OperationalState cluster implementation. The primary goal of removing the friend class declaration from the Delegate is achieved, which improves encapsulation and reduces coupling between Instance and Delegate. The introduction of VerifyOrDie in SetInstance adds valuable runtime checking to prevent incorrect state transitions. Furthermore, the changes to GetInstance correctly address a const-correctness issue, which is an excellent improvement for code quality and safety. Finally, updating the Instance destructor to nullify the pointer in the delegate is a good practice for lifecycle management, preventing potential dangling pointers. The changes are well-executed and improve the overall design.
|
PR #41204: Size comparison from 60d7fa3 to 1df646d Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
approved these changes
Oct 1, 2025
There was a problem hiding this comment.
Ok. In general though ... we have a public setter with a protected getter ... this feels like some odd design overall.
Will we lose most of this anyway or are we conflicting with #40804 ?
I would suggest to maybe consider waiting for the cluster to become code driven and then improve on that one.
|
PR #41204: Size comparison from db08deb to 85bca3f Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
@andy31415 Do you mean the architecture of the cluster code will change? |
|
PR #41204: Size comparison from 411e159 to c0b3243 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from dec325a to 50ef7bc Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
@bzbarsky-apple please confirm this PR to merge |
|
@andy31415 all CI/CD check passed, and you approved this pr, but still merging was blocked, so I sent you another request review. |
|
PR #41204: Size comparison from 1b2d06a to 15c1d03 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from 4df7a27 to 4fa3fd8 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from 7b16617 to a07c35e Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from f468d4e to af12101 Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from 9f8eb75 to 255cd46 Full report (34 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from 80b4fe1 to f00985d Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from e72c900 to 6812682 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from 5a37876 to 2ade783 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41204: Size comparison from 3108862 to 1e5e404 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
In my last PR (#40412), I added getter/setter methods to the Instance class and removed direct access to the variable. In this issue (#41093), I noticed there was an unnecessary friend class, which should be removed across all clusters. In this pull request, I fixed that bug in OperationalStateServer. Also, this change correctly fixes a const-correctness violation by providing proper const and non-const overloads for GetInstance. The previous implementation with the signature Instance * GetInstance() const was unsafe.
Related issues
Testing
There is no test because it's just a replacement.
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines