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

[#54] Rename char in types.rs to c_char. #68

Merged
merged 5 commits into from
Jan 6, 2024

Conversation

Shock-1
Copy link
Contributor

@Shock-1 Shock-1 commented Jan 2, 2024

  • Changes propogated

Notes for Reviewer

Look at errno.rs for the change in buffer type from char to c_char. Looks valid to me but that was the only involved change.

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Use either 'Closes #123' or 'Relates to #123' to reference the corresponding issue.

Closes #54

Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (26d979e) 77.97% compared to head (0fb8390) 78.02%.
Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   77.97%   78.02%   +0.05%     
==========================================
  Files         172      172              
  Lines       18729    18788      +59     
==========================================
+ Hits        14604    14660      +56     
- Misses       4125     4128       +3     
Files Coverage Δ
iceoryx2-bb/posix/src/directory.rs 62.18% <100.00%> (ø)
iceoryx2-bb/posix/src/group.rs 61.66% <100.00%> (ø)
iceoryx2-bb/posix/src/thread.rs 55.75% <100.00%> (ø)
iceoryx2-bb/posix/src/user.rs 77.95% <100.00%> (ø)
iceoryx2-pal/posix/src/linux/dirent.rs 100.00% <100.00%> (ø)
iceoryx2-pal/posix/src/linux/fcntl.rs 96.00% <100.00%> (ø)
iceoryx2-pal/posix/src/linux/mman.rs 81.13% <100.00%> (ø)
iceoryx2-pal/posix/src/linux/pthread.rs 86.27% <100.00%> (ø)
iceoryx2-pal/posix/src/linux/pwd.rs 100.00% <100.00%> (ø)
iceoryx2-pal/posix/src/linux/semaphore.rs 100.00% <100.00%> (ø)
... and 10 more

... and 17 files with indirect coverage changes

@elfenpiff
Copy link
Contributor

@Shock-1 awesome, thanks for the contribution! It looks like the preflight_check found some unformatted files, could you please call cargo fmt --all -- --check in the iceoryx2 root directory and format all files correctly. When this is done, I am happy to merge this PR.

@elfenpiff
Copy link
Contributor

elfenpiff commented Jan 2, 2024

@Shock-1 ouh I realized that every file in iceoryx2-pal/posix/src/$PLATFORM/*.rs must now use c_char as input argument instead of char. This is why the platforms macos and freebsd are failing.

But I think a simple search and replace should fix it.

I think you adjusted only the files in the linux platform.

@elfenpiff
Copy link
Contributor

elfenpiff commented Jan 2, 2024

@Shock-1 Ouh man, I should have known that platforms are always a little bit beastly when you tickle them.

Do you need a little bit of support here? I have all failing platforms running and could fix them on your branch.
I think in windows some files are still missing and for freebsd/macos we maybe need some more .cast() to cast to the right pointer type.

But I think the overall problem is that there are still some overlooked files in these platforms.

@Shock-1
Copy link
Contributor Author

Shock-1 commented Jan 2, 2024

Hmm, not having a machine with that OS is a bit blinding. I can at least test for Windows and then double-check.

@elBoberido
Copy link
Member

@Shock-1 there are still some files which use the old char type alias. Just grep the code base for char (there is a space before char) and there is a high chance it should be posix::c_char

@Shock-1
Copy link
Contributor Author

Shock-1 commented Jan 2, 2024

Will get back to it tomorrow after work. Thanks for all the help. Really looking forward to contributing more. Love to see more rust in my robotics.

@elfenpiff
Copy link
Contributor

@Shock-1 I would like to create a release soon and would love to include your changes. Do you think you have time to finish this PR soon?

@Shock-1
Copy link
Contributor Author

Shock-1 commented Jan 5, 2024

@Shock-1 I would like to create a release soon and would love to include your changes. Do you think you have time to finish this PR soon?

Yeah working on it. Windows is done. Just doing double-checks for Macos.

@Shock-1
Copy link
Contributor Author

Shock-1 commented Jan 5, 2024

@elfenpiff Take a look at it and merge it if it looks good.

@elfenpiff elfenpiff merged commit 33ab53b into eclipse-iceoryx:main Jan 6, 2024
21 checks passed
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.

Rename char in platform to c_char
3 participants