eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.68k stars 393 forks source link

iox-#2209 Refactor ssize_t to iox_ssize_t for Windows compatibility #2342

Closed khromenokroman closed 2 months ago

khromenokroman commented 2 months ago

This change updates the ssize_t type to iox_ssize_t in Windows-specific files to resolve a compatibility issue. It ensures consistent type usage and includes the necessary headers for type definitions.

Notes for Reviewer

Pre-Review Checklist for the PR Author

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

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

  1. [x] All open points are addressed and tracked via issues

References

@elBoberido I don't have windows to check the build :(

khromenokroman commented 2 months ago

Can you please add this to the end of the bugfix section of iceoryx-unreleased.md

ssize_t: redefinition; different basic types [#2209](https://github.com/eclipse-iceoryx/iceoryx/issues/2209)

@elBoberido fixed

khromenokroman commented 2 months ago

It also seems that you need to run clang-format on the changed files.

@elBoberido fixed

khromenokroman commented 2 months ago

@elBoberido done :) https://github.com/khromenokroman/iceoryx/actions/runs/10632957667

khromenokroman commented 2 months ago

Can you also please squash your commits?

@elBoberido done

khromenokroman commented 2 months ago

With the latest changes this should also be fixed.

@elBoberido if you return it then it doesn't work on msvc

elBoberido commented 2 months ago

With the latest changes this should also be fixed.

@elBoberido if you return it then it doesn't work on msvc

I just tried it locally with msvc and also mingw and it worked. I think it was just an intermediate problem. Can you revert that change to see if the CI has an other opinion :)

khromenokroman commented 2 months ago

I just tried it locally with msvc and also mingw and it worked. I think it was just an intermediate problem. Can you revert that change to see if the CI has an other opinion :)

@elBoberido canceled the changes

khromenokroman commented 2 months ago

With the latest changes this should also be fixed.

@elBoberido if you return it then it doesn't work on msvc

I just tried it locally with msvc and also mingw and it worked. I think it was just an intermediate problem. Can you revert that change to see if the CI has an other opinion :)

@elBoberido Error in Build & Test / build-test-windows-msvc

D:\a\iceoryx\iceoryx\iceoryx_platform\win\include\iceoryx_platform\mqueue.hpp(55,8): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [D:\a\iceoryx\iceoryx\build\hoofs\iceoryx_hoofs.vcxproj]
codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.22%. Comparing base (1549c96) to head (14d6e9b). Report is 2 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2342/graphs/tree.svg?width=650&height=150&src=pr&token=KWu8wdCc1S&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2342?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) ```diff @@ Coverage Diff @@ ## main #2342 +/- ## ========================================== - Coverage 78.23% 78.22% -0.01% ========================================== Files 433 433 Lines 16033 16033 Branches 2299 2299 ========================================== - Hits 12543 12542 -1 Misses 2647 2647 - Partials 843 844 +1 ``` | [Flag](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2342/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `78.05% <ø> (-0.01%)` | :arrow_down: | | [unittests_timing](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2342/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) | `15.06% <ø> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx#carryforward-flags-in-the-pull-request-comment) to find out more. [see 1 file with indirect coverage changes](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx/pull/2342/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)
elBoberido commented 2 months ago

With the latest changes this should also be fixed.

@elBoberido if you return it then it doesn't work on msvc

I just tried it locally with msvc and also mingw and it worked. I think it was just an intermediate problem. Can you revert that change to see if the CI has an other opinion :)

@elBoberido Error in Build & Test / build-test-windows-msvc

D:\a\iceoryx\iceoryx\iceoryx_platform\win\include\iceoryx_platform\mqueue.hpp(55,8): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [D:\a\iceoryx\iceoryx\build\hoofs\iceoryx_hoofs.vcxproj]

Oh, sorry. I was not precise enough. With revert the changes I actually meant to replace mqd_t with iox_ssize_t. Can you please do that for the two functions. With the current code it would be to replace ssize_t with iox_ssize_t.

Can you then also please squash the last three commits.

khromenokroman commented 2 months ago

Oh, sorry. I was not precise enough. With revert the changes I actually meant to replace mqd_t with iox_ssize_t. Can you please do that for the two functions. With the current code it would be to replace ssize_t with iox_ssize_t.

@elBoberido fixed

khromenokroman commented 2 months ago

Can you then also please squash the last three commits.

@elBoberido done

elBoberido commented 2 months ago

Thanks :)