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

[controller] replace reference of controller with pointer #2299

Closed

Conversation

Irving-cl
Copy link
Contributor

This PR changes the reference of Ncp::HostRcp inside all modules
to a pointer to Ncp::HostRcp. Instead of passing the reference of
Ncp::HostRcp to all modules in their contructors, this PR changes
it as passing a pointer in the Init of these modules.

This PR is also a subset of #2283. This change is required because:
Currently there is an instance of Ncp::HostRcp as a member of the
main class Application. Most modules inside Application have a
reference to the instance and the reference is passed to these modules
in their constructor (in Application's initialization list).
However, in the NCP case, Ncp::HostRcp shouldn't be contructed at
all. So updating the reference to pointer allows Application to not
initialize these modules in NCP case and initialize a different Host.

Copy link

codecov bot commented May 27, 2024

Codecov Report

Attention: Patch coverage is 82.14286% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 39.94%. Comparing base (2b41187) to head (9786890).
Report is 668 commits behind head on main.

Files Patch % Lines
src/sdp_proxy/discovery_proxy.cpp 45.45% 6 Missing ⚠️
src/trel_dnssd/trel_dnssd.cpp 50.00% 5 Missing and 1 partial ⚠️
src/border_agent/border_agent.cpp 75.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2299       +/-   ##
===========================================
- Coverage   55.77%   39.94%   -15.84%     
===========================================
  Files          87       90        +3     
  Lines        6890     9811     +2921     
  Branches        0      722      +722     
===========================================
+ Hits         3843     3919       +76     
- Misses       3047     5693     +2646     
- Partials        0      199      +199     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Irving-cl Irving-cl force-pushed the refactor_controller_init branch 5 times, most recently from d33cc77 to dcfb339 Compare May 27, 2024 06:35
@Irving-cl Irving-cl marked this pull request as ready for review May 27, 2024 06:45
@Irving-cl Irving-cl force-pushed the refactor_controller_init branch 9 times, most recently from 9874c00 to 2fae7a1 Compare May 27, 2024 12:19
Copy link
Contributor

@superwhd superwhd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the two-step initialization (constructor + Init()) is a bit more error-prone than the one-step initialization.

What about refactor it in a different way: Define interfaces for each component and let the RCP-based implementation store the reference to the RcpHost while the NCP one doesn't.

@Irving-cl Irving-cl force-pushed the refactor_controller_init branch 2 times, most recently from e0fe0f4 to 72f51dd Compare May 28, 2024 01:54
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.

2 participants