Iox 1640 create polymorphic singleton abstraction

[MatthiasKillat]

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 messages are signed (git commit -s)
   • 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

refer to https://github.com/eclipse-iceoryx/iceoryx/pull/1656/files#diff-b342f851e544aab4457e99291a6b66baad23e941cfb95dbc4454bf08669dc356 for explanation of the functionality

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 Create exchangeable singleton abstraction #1640

[MatthiasKillat]

I need to port the clang format CI fix from the other PR to this one. The other PR will likely not be merged soon, but @elBoberido proposed how to build a singleton manager on top as well to solve the singleton destruction order problem.

This will become important for posh and hopefully be a useful building block.

The current PR is for error handling and logging.

[elfenpiff]

Please

• use include guards instead of pragma once
• add the copyright header
• move implementation into inl file
• use noexcept on every method
• do not use std::cerr, use Logger instead
• do not use std::terminate, use cxx::Ensures instead

[MatthiasKillat]

@elfenpiff The restructuring will be done.

However, we should not have the default implementation depend on logger or later error handler. We could have empty default behavior instead of terminate, the hook is supposed to be set somehow by the concrete logger or error handler.

Some methods can be noexcept, but not all I think. Need to evaluate generics. I only will add it where we can provide a no-throw guarantee.

[codecov]

Codecov Report

Merging #1656 (b35cb02) into master (5f4dd12) will increase coverage by 0.07%.
The diff coverage is 92.64%.

❗ Current head b35cb02 differs from pull request most recent head fcf639d. Consider uploading reports for the commit fcf639d to get more accurate results

@@            Coverage Diff             @@
##           master    #1656      +/-   ##
==========================================
+ Coverage   75.42%   75.49%   +0.07%     
==========================================
  Files         377      379       +2     
  Lines       14619    14693      +74     
  Branches     2084     2098      +14     
==========================================
+ Hits        11026    11093      +67     
  Misses       2961     2961              
- Partials      632      639       +7     

Flag	Coverage Δ
unittests	75.16% <92.64%> (+0.07%)	⬆️
unittests_timing	15.28% <23.52%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
...fs/internal/design_pattern/polymorphic_handler.inl	91.89% <91.89%> (ø)
.../internal/design_pattern/static_lifetime_guard.inl	93.54% <93.54%> (ø)
...sh/include/iceoryx_posh/internal/popo/wait_set.inl	91.47% <0.00%> (-1.22%)	⬇️

[MatthiasKillat]

Refactored and added some commits of @dkroenke to update clang-format.

[MatthiasKillat]

I made all methods noexcept, even if formally no nothrow guarantee can be given for almost all of them.

I am still not sure how Axivion treats this. I think if it cannot prove that everything is noxexcept in the implementation it does not care (function can be noexcept but does not have to).

[elBoberido]

Just a quick first review

[MatthiasKillat]

@elBoberido I adressed the following issues.

1. Use of mutex - not required anymore but it will block during concurrent instance construction (only one call is allowed to access the memory to construct), there is no way around it with limited memory to construct the instance (but it will always return with fair scheduling, as it would with mutex)
2. ensure the lifetime of the handlers to be set by enforcing the use of a StaticLifetimeGuard
3. The StaticLifetimeGuard essentially generalizes/adapts the nifty counter pattern but provides arguably better encapsulation and easier usage

This introduces complexity in the implementation but I think they are useful buildings blocks for recurring problems. I placed them in design_patterns (because they are design_patterns) but I am not very attached to this name (could be a building block, whatever).

[elBoberido]

I think this is now in good condition as far as the functionality is concerned but I'm a bit skeptical about the API. Unfortunately I don't have a better alternative.

[MatthiasKillat]

@elBoberido I am not perfectly happy with the API either, but probably in other ways than you. The guards are good internally but using them in the API limits functionality in ways I did not want initially (partly related to the bool return change` and more.

Nevertheless it solves a problem in a useful way and we can build on that, e.g. I will for error handler exchange.

[elBoberido]

I guess this does not restrict other hooks to define a non-noreturn method?

[MatthiasKillat]

No it does not and should not. It is not mandatory for any hook. It is just plugged in as a template argument type ...

However, it is a guarantee the DefaultHooks are able to give and so they should. Can remove it, but would advise not to. This allows optimization and communicates intent. And is portable in C++11.

outdated