Iox #261 Replace std::terminate with Expects/Ensures

[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. Branch follows the naming format (iox-#123-this-is-a-branch)
4. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit messages are signed (git commit -s)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
5. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
6. Relevant issues are linked
7. Add sensible notes for the reviewer
8. All checks have passed (except task-list-completed)
9. Assign PR to reviewer

Notes for Reviewer

• Replace std::terminate with Expects/Ensures or errorHandler()
• Resolve some cyclic dependencies
  • Move makeScopedStatic() to its own hpp / inl

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
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Closes Clean-up std::terminate usage #261

[codecov]

Codecov Report

Merging #750 (6e8ad63) into master (4ce5310) will increase coverage by 0.19%.
The diff coverage is 34.48%.

@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
+ Coverage   74.03%   74.22%   +0.19%     
==========================================
  Files         319      320       +1     
  Lines       11426    11416      -10     
  Branches     1972     1973       +1     
==========================================
+ Hits         8459     8474      +15     
+ Misses       2192     2165      -27     
- Partials      775      777       +2     

Flag	Coverage Δ
unittests	73.04% <34.48%> (+0.22%)	⬆️
unittests_timing	30.55% <22.41%> (+0.05%)	⬆️

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

Impacted Files	Coverage Δ
iceoryx_posh/source/mepoo/mepoo_config.cpp	74.19% <0.00%> (ø)
...osh/source/roudi/application/iceoryx_roudi_app.cpp	30.76% <ø> (ø)
...eoryx_utils/include/iceoryx_utils/cxx/helplets.hpp	82.50% <ø> (+5.75%)	⬆️
iceoryx_utils/include/iceoryx_utils/cxx/vector.hpp	100.00% <ø> (ø)
...clude/iceoryx_utils/internal/cxx/scoped_static.inl	0.00% <0.00%> (ø)
iceoryx_utils/source/posix_wrapper/mutex.cpp	87.09% <0.00%> (+12.09%)	⬆️
iceoryx_posh/source/roudi/process_manager.cpp	61.44% <16.66%> (-0.26%)	⬇️
..._utils/include/iceoryx_utils/internal/cxx/list.inl	91.63% <30.76%> (+1.59%)	⬆️
iceoryx_posh/source/runtime/posh_runtime.cpp	65.86% <33.33%> (+0.90%)	⬆️
...oryx_utils/source/posix_wrapper/access_control.cpp	56.55% <33.33%> (+0.35%)	⬆️
... and 14 more

[elfenpiff]

Please try to avoid the usage of cxx::Expects and cxx::Ensures as much as possible. I think they should only be used when you encounter an error from which you can not recover or where it is impossible for you to continue.

Please try to use cxx::expected as often as possible. The reasons are:

• the compiler will force the user to perform some error handling. When you use cxx::Expects/cxx::Ensures the user will most likely be not aware that the program can terminate under certain conditions.
• If you use cxx::expected the error is baked into the signature and documented
• When you use cxx::Expects/Ensures you have to document this in a doxygen note or somewhere in the brief section. This can be easily be overlooked, by us when we refactor the code, by the user when they use the code.

The last point is demonstrated well in this PR, since in most of the doxygen docu it is nowhere mentioned that the methods which are using cxx::Expects/cxx::Ensures can be lead to fatal process termination.

[elfenpiff]

Nice! Thanks, the platform correction has to be always the last include to cleanup the mess which other platforms produce.

[elBoberido]

since iceoryx is a library, are we sure we don't break others code with this? I also do not have a better solution for this, except not using most of the stuff in that header within iceoryx

[elfenpiff]

This is wrong. It was created today.

I know the first implementation was in 2019 in the helplets.hpp file but thanks to git the whole history is preserved and correct. Here we have some implicit assumptions which just lead directly into a weird and complex world.

[mossmaurice]

Sorry but copyright does not rely on the Git history. This file was copied and pasted and not written by me. As a result all copyright information has to be copied to new file as well. It's done like this in every FOSS project I've worked on.

[elfenpiff]

@mossmaurice I strongly disagree. @elBoberido , @dkroenke , @budrus @FerdinandSpitzschnueffler @MatthiasKillat what do you think?

The reason is the following. Let's assume you are right then everytime we do some refactoring and move a function into a new file we have to trace the whole function. This means when it was first written in file A, then moved to file B, then to file C and with the new refactoring to file D we have to trace the git history of the function through the files. In the new file we have then to add all the people who ever touched a piece of the function in the copyright header.

It is getting even worse when we would like to split up the function in the next refactoring in to different files. Then we have to trace every line!!! With every file we create and move code we have trace all the lines where they did come from! This is absolutely horrible.

Please do not close or resolve the conversation until we all agreed on how we proceed in such cases.

[dkroenke]

@elfenpiff @mossmaurice
Well, looking at the Eclipse Handbook could maybe clarify this: https://www.eclipse.org/projects/handbook/#legaldoc-faq

How do file headers change when content is refactored?

When you split a file during a refactoring exercise, any new files that you create must automatically inherit the header of the original. If even fragment of text from a single line of the original content survives the refactoring (whether generated, rote, or otherwise), then the original copyright statement and author information should be retained.

Even a refactoring exercise that does not change behaviour, may add intellectual property. When refactoring adds content, copyright statements should be updated to reflect the addition. In concrete terms, you can either add the new copyright holders to the existing copyright statement or append "and others" (especially when the list of copyright holders gets long).

[mossmaurice]

Ok, thanks @dkroenke for clarifying this. If we should get into a situation with dozens of copyright holder we might switch to "and others". We can raise awareness about this topic in the next dev meetup.

[elfenpiff]

@mossmaurice @dkroenke How do we ensure the correctness of the copyright headers as a reviewer? Do we have to retrace the code lines and functions over the total git history? Or anyone else?

At the moment we can handle this somehow but when there will be more this is the ultimate time killer.

[elBoberido]

nitpicking, fatalError could be anything and if someone extends this method with another fatal error that Person could accidentally also use this flag even if it's a totally different error. How about using a lambda

auto handlePreconditionNotFulfilled = [] (bool preconditionNotFulfilled, const char* message) {
    if (preconditionNotFulfille)
    {
    LogFatal() << message;
        /// @todo #539 Use separate error enums once RouDi is more modular
        errorHandler(Error::kROUDI__PRECONDITIONS_FOR_PROCESS_MANAGER_NOT_FULFILLED, nullptr, ErrorLevel::FATAL);
    }
};

handlePreconditionNotFulfilled(!maybeSegmentManager.has_value(), "Invalid state! Could not obtain SegmentManager!");

[mossmaurice]

I see your point, but IMHO the maintenance effort is not much higher, hence I'll stick with the current approach as this will be refactored relatively soon anyway.

[elBoberido]

it's your decision, but keep in mind, nothing is more definitive than the temporary ;)

[elBoberido]

since iceoryx is a library, are we sure we don't break others code with this? I also do not have a better solution for this, except not using most of the stuff in that header within iceoryx

[elfenpiff]

Please place the termination comments directly into the function/method documentation and then you get my thumbs up :)