Ensure header files and .cpp files use the codal namespace appropriately #437
Conversation
This file contains 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
carlosperate
commented
Jul 23, 2024
| @@ -31,7 +31,6 @@ DEALINGS IN THE SOFTWARE. | |||
| #include "CodalDevice.h" | |||
| #include "ErrorNo.h" | |||
| #include "NRFLowLevelTimer.h" | |||
| #include "Matrix4.h" | |||
There was a problem hiding this comment.
Removed as it wasn't used.
carlosperate
commented
Jul 23, 2024
Comment on lines
-45
to
-47
| namespace MicroBitUtility | ||
| typedef enum replyState_t | ||
| { | ||
| typedef enum replyState_t |
There was a problem hiding this comment.
This enum is declared and only used inside this cpp file, so no need to declare it inside the MicroBitUtility namespace, otherwise with these changes each usage in this file would have to be prepended by the namespace.
carlosperate
force-pushed
the
namespaces
branch
2 times, most recently
from
977d285
to
9f3d96e
Compare
carlosperate
added a commit
to carlosperate/codal-nrf52
that referenced
this pull request
Jul 24, 2024
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 header file to set up this namespace 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
carlosperate
added a commit
to carlosperate/codal-nrf52
that referenced
this pull request
Jul 24, 2024
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 header file to set up this namespace 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
carlosperate
added a commit
to carlosperate/codal-nrf52
that referenced
this pull request
Jul 24, 2024
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
pushed a commit
to lancaster-university/codal-nrf52
that referenced
this pull request
Jul 24, 2024
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
…ely. Also update the codal-core and codal-nrf52 dependencies as they include changes on the same topic. The classes declared in the Mbed compatibility header files have not been wrapped in the codal namespace, as programmes including the mbed.h header might not expect them to be inside that namespace. This header file is not included in any of the codal-microbit-v2 or dependencies, it's mostly used by MakeCode extensions that were accessing Mbed-specific libraries in micro:bit V1.
microbit-carlos
approved these changes
Jul 24, 2024
microbit-carlos
merged commit 762629c
into
lancaster-university:master
13 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is part of:
using namespace codalshouldn't be used in header files #240The only thing that hasn't been added to the codal namespace are the Mbed header files, as those are only included by programmes explicitly doing
include <mbed.h>:codal-microbit-v2/inc/compat/mbed.h
Lines 11 to 17 in 96faa85
This PR uses a new config option added to codal-core in lancaster-university/codal-nrf52@8802eb4. By default is set to
1, so that the codal namespace is still added globally in MicroBit.h, but the user can change this config option in case they need to integrate CODAL with other frameworks and avoid clashes in the namespace.