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

Replace using namespace in headers with explicit namespaces. #43

Merged

Conversation

carlosperate
Copy link
Contributor

This PR is one part to resolve issue lancaster-university/codal-microbit-v2#240:

To avoid namespace pollution headers should not use using namespace codal;, and instead it can be added into any .cpp files that needed (all of them in this repo).

I still need to test this PR in isolation with a normal CODAL build, so I'll leave this as a draft PR until I've finished that.

@carlosperate
Copy link
Contributor Author

carlosperate commented Jan 1, 2023

Okay, had to leave the using namespace codal in the NRF52PWM.h header file, as some .cpp files in codal-microbit-v2 depended on it. Once we fixed all the namespacing in codal-microbit-v2 we can remove it from NRF52PWM.h in a different PR.

I think this approach, keeping some of the namespace in this PR so that is safe to merge, even if it means more PRs in total, is probably a bit easier to manage than orchestrating several PRs to be merged in order.

@carlosperate
Copy link
Contributor Author

carlosperate commented Jul 22, 2024

PR #54 with a partial implementation of this has been merged already, and this branch has been rebased.

I'll change this PR to "draft" status as it shouldn't be implemented until other CODAL targets that depend on codal-nrf52 are updated first with a patch similar to bdfa1ec.

@carlosperate carlosperate marked this pull request as draft July 22, 2024 18:27
@carlosperate carlosperate force-pushed the namespaces branch 2 times, most recently from 7470f39 to 4a4cdba Compare July 24, 2024 09:24
Flag introduced in codal-core in:
lancaster-university/codal-core@509086c

By default the `using namespace` line would still be present, as
removing it can cause some codal targets to break if they depend
on the namespace to be set up globally.

For example, the codal-microbit-v2 target had this update to
fix the namespacing:
lancaster-university/codal-microbit-v2#437

This patch is part of:
lancaster-university/codal-microbit-v2#240
@microbit-carlos
Copy link
Contributor

microbit-carlos commented Jul 24, 2024

Okay, so there is quite a few CODAL targets that might depend on some of these header files in codal-nrf52 setting codal to the global namespace, and making this change can be disruptive to them.

And there are a few codal-microbit-v2 forks with enough changes where even applying lancaster-university/codal-microbit-v2#437 could create conflicts:

So, a codal config flag has been added to codal-core in lancaster-university/codal-core#171, to be able to include these using namespace codal lines as configurable option that by default leaves them there.

This still means that all those CODAL targets that update to the latest codal-nrf52 also need to make sure they update to the latest codal-core, but that should be the process anyway. And for codal-microbit-v2 forks, pulling from upstream should be enough for them to be okay.

@carlosperate carlosperate marked this pull request as ready for review July 24, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants