Iox #391 static memory alternative for std function

[MatthiasKillat]

Pre-Review Checklist for the PR Author

1. Branch follows the naming format (iox-#123-this-is-a-branch)
2. 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)
3. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
4. Relevant issues are linked
5. Add sensible notes for the reviewer
6. All checks have passed
7. Assign PR to reviewer

Notes for Reviewer

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

Post-review Checklist for the Eclipse Committer

1. All checkboxes in the PR checklist are checked or crossed out
2. Merge

References

• Closes static memory alternative for std::function #391

Summary

Added a static storage alternative for std::function including tests. This is supposed to store any callable up to a given storage size. As is also the case for sd::function, it cannot e used in shared memory (this cannot be avoided due to the fact that pointers to e.g. free functions but also lambdas are stored which will usually not be valid in another process).

The implementation uses cxx::function_ref, otherwise parts of those functionality would have been rewritten/duplicated. It is important to note that much of the complications arise due to the fact that information about the underlying stored type is lost but it still must be ensured that it is copied/moved properly (according to RAII). Naturally these problems do NOT arise when only a reference is stored (as in function_ref).

Fixed an issue that function_ref was not constructible from function pointers of free functions (only function names, but those are not function pointers but are converted to them). There is still an open point of a conversion from something potentially not convertible to a void pointer in the function_ref ctor (existed before as well, as it would take a universal reference to anything callable). For function_pointers this is compliant with the POSIX standard, but in general it is not (function pointers cannot be converted to void pointers in general).

This requires some further investigation. A potential fix could be added here as well.

[mossmaurice]

Only reviewed function_ref and type_trait changes so far.

[orecham]

Some punctuation would help with readability.

[orecham]

Idea looks sound to me. Just needs some documentation !

[MatthiasKillat]

@ithier. I will improve the documentation. I prefer brief docu though, unless the use cases are very tricky. No one reads long docu for functions or classes. :-)

[mossmaurice]

Great work! Tests are not yet reviewed.

1. Please check if the implementation of storable_function can be simplified by using one generic c'tor, as in std::function:

template< class F, class Alloc >
function( std::allocator_arg_t, const Alloc& alloc, F f );

2. Align cxx::PoorMansHeap usage

[MatthiasKillat]

I will update the PR in a day or two, until then no further review needed.

[MatthiasKillat]

Reworked, can be reviewed now.

[elfenpiff]

Only partially reviewed I will continue on monday.

Please adjust all copyright headers.

[MatthiasKillat]

The pipeline reports a problem with alignof that is used in a non-standard way - applied to expressions (gcc allows this). I have changed this to be semantically equivalent for our case...

Also needed to remove a test for the static storage which essentially checked that deallocation does not actually clear the storage but to do so caused an uninitialized read warning (unavoidable in this case I think). Test removed since this is no real requirement for functionality, just an aspect of optimization (i.e. not setting the memory to zero as most allocators do).

Stale review, changed quite a bit and addressed all points.

[MatthiasKillat]

@elfenpiff @mossmaurice I think I addressed all the points and the CI is green.

It is by no means perfect, but I think it is mature enough to be tried out internally. Should there be any further issues I will find them when I replace internal std::function occurrences next. Usage should be fine (according to tests), within the limitations of the capacity constraint and behavior of operator() which should be as std::function.

[mossmaurice]

@elfenpiff @mossmaurice I think I addressed all the points and the CI is green.

It is by no means perfect, but I think it is mature enough to be tried out internally. Should there be any further issues I will find them when I replace internal std::function occurrences next. Usage should be fine (according to tests), within the limitations of the capacity constraint and behavior of operator() which should be as std::function.

👍 I'll finish off the review tonight.

Could you merge origin/master to this branch? There are still some conflicts.

[mossmaurice]

10/13 done

[mossmaurice]

LGTM in general, only minor findings. Let's get this merged!

[mossmaurice]

Another thing that just popped up: As this PR introduced a lot of new code, could you run Axivion with these coding rules over this PR (soon it will run with every PR)?

[MatthiasKillat]

No, in my opinion we will introduce Axivion for the whole code base and from then on we will run it for every PR. I have no license for it yet and do not see that we should block the PR due to this. I will fix any problem occurring due to this in a new PR, but we should introduce Axivion properly before this is done.

[MatthiasKillat]

Now it should be ok if the CI builds, wanted to get rid off one merge commit at least hence the force pushes.

[mossmaurice]

I will fix any problem occurring due to this in a new PR, but we should introduce Axivion properly before this is done.

Could you create an issue for that? Axivion was introduced in #409 for the master branch, but currently lacks a script for the local scan.

[MatthiasKillat]

Before we create any issue we should wait whether anything pops up at all.

[MatthiasKillat]

@elfenpiff @mossmaurice should be ready to merge