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

Forward declaration "class DeviceCommissioner" in src/controller hides a build dependency #10104

Closed
vivien-apple opened this issue Sep 30, 2021 · 3 comments
Labels
stale Stale issue or PR V1.X

Comments

@vivien-apple
Copy link
Contributor

Why is the forward declaration required? I am concerned when these show up since it may hint that some circular dependencies or wrong build dependencies are being introduced (i.e. we can use this to bypass gn enforcement of library dependencies).

please include the header instead.

Originally posted by @andy31415 in #9847 (comment)

@vivien-apple
Copy link
Contributor Author

Here is my reply from the original issue.

You are correct in the sense that CHIPDeviceController.h includes SetUpCodePairer.h and SetupCodePairer.h declares a member of type DeviceCommissioner which is defined into CHIPDeviceController.h.

As I mentioned in the initial comment of the issue, all the "mdns" operations are tied to CHIPDeviceController, and SetupCodePairer uses it... It is unclear that this mdns code needs to be tied to CHIPDeviceController directly.

As such I could have added a delegate to call back into DeviceCommissioner::PairDevice(NodeId remoteId, RendezvousParameters & params) instead of doing mCommissioner->PairDevice directly but since I do need to have a reference to an instance of the commissioner already for mdns operations I have used it...

I think the real fix here would be to move the mdns operations to a separate class, and have it beeing used from multiple classes.

@stale
Copy link

stale bot commented Sep 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Sep 16, 2022
@stale
Copy link

stale bot commented Oct 1, 2022

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issue or PR V1.X
Projects
None yet
Development

No branches or pull requests

2 participants