lancaster-university / codal-nrf52

MIT License
4 stars 15 forks source link

Replace `using namespace` in headers with explicit namespaces. #43

Closed carlosperate closed 4 months ago

carlosperate commented 1 year ago

This PR is one part to resolve issue https://github.com/lancaster-university/codal-microbit-v2/issues/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 commented 1 year ago

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 commented 4 months ago

PR https://github.com/lancaster-university/codal-nrf52/pull/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 bdfa1eca85dbbd6fc8aaf5fc4c3fd00c1f65762d.

microbit-carlos commented 4 months ago

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 https://github.com/lancaster-university/codal-microbit-v2/pull/437 could create conflicts:

So, a codal config flag has been added to codal-core in https://github.com/lancaster-university/codal-core/pull/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.