iox-1391 Move time-related classes to Time module

[mossmaurice]

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
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
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. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
11. Assign PR to reviewer

Notes for Reviewer

• Move deadline_timer, adaptive_wait and Duration to Time module
• Remove 2nd level namespace cxx
• Create legacy header for backwards compatibility

Checklist for the PR Reviewer

• 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

• Closes Split iceoryx_hoofs into logical modules #1391 (Partly)

[codecov]

Codecov Report

Merging #1863 (a264a65) into master (e832ccb) will increase coverage by 0.00%.
The diff coverage is 83.33%.

@@           Coverage Diff           @@
##           master    #1863   +/-   ##
=======================================
  Coverage   74.97%   74.97%           
=======================================
  Files         383      383           
  Lines       14692    14692           
  Branches     2099     2099           
=======================================
+ Hits        11015    11016    +1     
  Misses       3031     3031           
+ Partials      646      645    -1     

Flag	Coverage Δ
unittests	74.64% <58.33%> (+<0.01%)	⬆️
unittests_timing	15.28% <50.00%> (-0.02%)	⬇️

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

Impacted Files	Coverage Δ
.../include/iceoryx_dust/posix_wrapper/named_pipe.hpp	100.00% <ø> (ø)
iceoryx_dust/source/posix_wrapper/named_pipe.cpp	51.53% <0.00%> (ø)
...oofs/internal/posix_wrapper/unix_domain_socket.hpp	100.00% <ø> (ø)
iceoryx_hoofs/source/units/duration.cpp	92.85% <ø> (ø)
iceoryx_hoofs/time/include/iox/detail/duration.inl	94.47% <ø> (ø)
...nternal/popo/building_blocks/chunk_distributor.hpp	100.00% <ø> (ø)
...eoryx_posh/internal/runtime/ipc_interface_base.hpp	100.00% <ø> (ø)
...yx_posh/internal/runtime/ipc_runtime_interface.hpp	100.00% <ø> (ø)
...clude/iceoryx_posh/roudi/roudi_cmd_line_parser.hpp	100.00% <ø> (ø)
iceoryx_posh/source/roudi/roudi.cpp	58.44% <0.00%> (ø)
... and 6 more

[elfenpiff]

No please do not name this Time it is a Units module. Otherwise we will create in the near future a lot of additional tiny modules with one or two classes of content.

It may makes sense that in this Units module is later also a unit for velocity, acceleration etc. for our users or we have an abstraction for of the unit memory

[elfenpiff]

No please do not name this Time it is a Units module. Otherwise we will create in the near future a lot of additional tiny modules with one or two classes of content.

It may makes sense that in this Units module is later also a unit for velocity, acceleration etc. for our users or we have an abstraction for of the unit memory

@mossmaurice The high level classes like adaptive_wait etc. would not be stored in a units module but here I would try to find something more generic than Time.

[mossmaurice]

@elfenpiff Thanks for your review and remarks.

No please do not name this Time it is a Units module. Otherwise we will create in the near future a lot of additional tiny modules with one or two classes of content.

It may makes sense that in this Units module is later also a unit for velocity, acceleration etc. for our users or we have an abstraction for of the unit memory

I remember we had this discussion in #1600 during October 2022. I can definitely see your point that Units is something different and might be extended later. Back in Oct we concluded that, as you mentioned, in order to keep the number of overall modules as low as possible, we will not further differentiate and just use one module for all time- and units-related classes. IMHO that's still a good idea and we should not change this decision. I would be ok with renaming the current module to something like Units and time utilities. The STL has a similar module Date and time utilites, which contains the clocks, time points, duration, etc. What do you think?

@elBoberido What's your take? Shall we change the plan?

[elfenpiff]

@elBoberido What's your take? Shall we change the plan?

@mossmaurice Let it be Time then. In the readme you linked there we also concluded that every posix construct is stored inside the posix folder and that we there replicate the component structure.
Duration is a posix abstraction (see timeval, timespec and clock_gettime) and belongs therefore in posix/time but I would also agree to simply posix

The deadline timer uses the duration and has no posix dependency and can therefore be stored simply in time.

The adaptive wait is a little bit more complicated. It uses std::this_thread::yield() and std::this_thread::sleep_for calls which are not allowed for us in the final implementation and they have to be replaced with the posix calls sched_yield and nanosleep.
So I would recommend to move adaptive_wait into posix/time since this is the correct final resting place for it. If you have time you can use the posix call and replace std::this_thread::yield() and std::this_thread::sleep_for for the previously mentioned calls - then it is cleaned up correctly. For me it makes sense since those are only 3 additional line changes.

[mossmaurice]

@elfenpiff Got it. It's now as follows:

1. Classes which depend on a low-level OSI API go into posix, I would like to rename this folder at some point after v3.0

posix/time/include/iox/deadline_timer.hpp
posix/time/include/iox/detail/adaptive_wait.hpp
posix/time/source/adaptive_wait.cpp
posix/time/source/deadline_timer.cpp

2. Classes which only depend on allowed STL headers go into the iceoryx_hoofs/$MODULE_NAME

time/include/iox/duration.hpp
time/include/iox/detail/duration.inl

I'll carve out the std::chrono, timespec and timeval-dependecies into iceoryx_dust like std_string_support.hpp on a follow-up PR.

[elBoberido]

@mossmaurice while chrono can be moved to dust, timespec and timeval must remain in hoofs since it is our native way to convert a Duration to a time value for POSIX calls like sem_timedwait. These could potentially be moved to 'posix/time/whatever.hpp'

[mossmaurice]

@elBoberido

while chrono can be moved to dust, timespec and timeval must remain in hoofs since it is our native way to convert a Duration to a time value for POSIX calls like sem_timedwait. These could potentially be moved to 'posix/time/whatever.hpp'

That's true, I'll address that on the follow-up PR.