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

iox-#2327 Replace macro ENUM with IOX_ENUM and CLASS with IOX_CLASS #2328

Conversation

mikebentley15
Copy link
Contributor

@mikebentley15 mikebentley15 commented Aug 19, 2024

Notes for Reviewer

The macros ENUM and CLASS are very generic names and can potentially cause naming conflicts with user code or other third-party libraries. These macros are intended to make the headers compatible with both C and C++. The ENUM macro is unnecessary since the enum prefix for variable declarations is required in C and optional in C++. The CLASS macro is required to match the Microsoft C++ ABI -- defined as class when in C++ mode and struct when in C mode.

This PR seeks to solve this by

  • Removing the ENUM macro, replacing its use with the enum keyword.
  • Rename CLASS macro to IOX_C_CLASS to reduce likelihood of naming conflicts
    • Command: sed -i -e 's/\<CLASS\>/IOX_C_CLASS/g' $(find -type f)
    • One file was reverted since CLASS was in a comment; not referencing the macro.

See issue #2327 for a minimal example to reproduce the issue and existing workarounds.

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. Assign PR to reviewer

Checklist for the PR Reviewer

  • Consider a second reviewer for complex new features or larger refactorings
  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@mikebentley15
Copy link
Contributor Author

After this review goes through, I may make another PR to redo this change for version v2.90 to integrate with our pipeline.

@mikebentley15 mikebentley15 force-pushed the iox-2327-rename-enum-and-class-macros-to-unique-names branch from 62e4395 to c5c7fe6 Compare August 19, 2024 23:17
@elBoberido
Copy link
Member

After this review goes through, I may make another PR to redo this change for version v2.90 to integrate with our pipeline.

There is no branch for v2.90. It is only a tag and therefore there is nothing a PR could be merged to. If you need this for your internal pipeline, you need to either carry a patch or update to the latest main branch.

@mikebentley15 mikebentley15 force-pushed the iox-2327-rename-enum-and-class-macros-to-unique-names branch 2 times, most recently from cfc3445 to 4523cb2 Compare August 21, 2024 18:10
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.23%. Comparing base (912931a) to head (21f6d09).
Report is 23 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2328      +/-   ##
==========================================
+ Coverage   78.07%   78.23%   +0.15%     
==========================================
  Files         432      433       +1     
  Lines       15920    16034     +114     
  Branches     2303     2302       -1     
==========================================
+ Hits        12430    12544     +114     
  Misses       2644     2644              
  Partials      846      846              
Flag Coverage Δ
unittests 78.05% <100.00%> (+0.15%) ⬆️
unittests_timing 15.03% <33.33%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ceoryx_binding_c/source/c2cpp_enum_translation.cpp 100.00% <100.00%> (ø)
iceoryx_binding_c/source/c_listener.cpp 85.45% <100.00%> (ø)
iceoryx_binding_c/source/c_service_discovery.cpp 98.27% <ø> (ø)
iceoryx_binding_c/source/c_wait_set.cpp 90.22% <100.00%> (ø)

... and 16 files with indirect coverage changes

@mikebentley15 mikebentley15 force-pushed the iox-2327-rename-enum-and-class-macros-to-unique-names branch from edc7718 to a5f5a64 Compare August 23, 2024 20:57
@mikebentley15 mikebentley15 force-pushed the iox-2327-rename-enum-and-class-macros-to-unique-names branch from a5f5a64 to 21f6d09 Compare August 23, 2024 21:07
@elBoberido elBoberido merged commit a285e98 into eclipse-iceoryx:main Aug 27, 2024
25 checks passed
@elBoberido
Copy link
Member

@mikebentley15 thanks very much for your contribution. I think I already mentioned it above, we are currently working on iceoryx2 which is written in Rust and will have C and C++ bindings with the next release. We will also present it in the iceoryx developer meetup next week on Tuesday. We would also be happy to hear what you are using iceoryx for :)

@mikebentley15 mikebentley15 deleted the iox-2327-rename-enum-and-class-macros-to-unique-names branch August 29, 2024 01:41
@mikebentley15
Copy link
Contributor Author

@elBoberido, you're welcome! It's pretty cool that you have iceoryx2 implemented in Rust. I'm using iceoryx indirectly via Cyclone DDS. Cheers!

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.

Compilation Conflict with Apache Arrow from ENUM macro
2 participants