iox-1750 Add Expects to cxx::expected::value() and get_error()

[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

• Add Expects to cxx::expected::value() and get_error()
• Fix legacy coding rule and add two doxygen comments

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 Accessing cxx::expected which contains a value or error leads to UB if the contrary is accessed  #1750

[codecov]

Codecov Report

Merging #1754 (434d0ce) into master (8f6af2f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1754   +/-   ##
=======================================
  Coverage   77.37%   77.38%           
=======================================
  Files         366      366           
  Lines       14187    14196    +9     
  Branches     1983     1983           
=======================================
+ Hits        10977    10985    +8     
  Misses       2583     2583           
- Partials      627      628    +1     

Flag	Coverage Δ
unittests	77.03% <100.00%> (+<0.01%)	⬆️
unittests_timing	15.70% <48.00%> (+0.07%)	⬆️

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

Impacted Files	Coverage Δ
...eoryx_hoofs/include/iceoryx_hoofs/cxx/expected.hpp	100.00% <ø> (ø)
...fs/include/iceoryx_hoofs/internal/cxx/expected.inl	98.34% <100.00%> (+0.13%)	⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp	85.71% <0.00%> (-2.86%)	⬇️

[elfenpiff]

@mossmaurice the issue itself is based on a false assumption. Accessing a non existing value or error in an expected is defined behavior.
The underlying variant returns nullptr in get_type_at_index which is used in the expected implementation.

So accessing a non existing value or error is defined as dereferencing a nullptr.

The question which now remains: should we call the error handler and where. Since this is not a problem of the expected but the underlying variant I suggest to adjust the methods there. We could adjust get_type_at_index so that it does no longer return a nullptr when the type is not emplaced there but calls the error handler.

What do you think?

[mossmaurice]

the issue itself is based on a false assumption. Accessing a non existing value or error in an expected is defined behavior. The underlying variant returns nullptr in get_type_at_index which is used in the expected implementation.

So accessing a non existing value or error is defined as dereferencing a nullptr.

The question which now remains: should we call the error handler and where. Since this is not a problem of the expected but the underlying variant I suggest to adjust the methods there. We could adjust get_type_at_index so that it does no longer return a nullptr when the type is not emplaced there but calls the error handler.

What do you think?

@elfenpiff

I was thinking about this before. Looking at the STL API of std::variant I came to the conclusion, that returning a nullptr and putting the burden on the user to check it, is the best we can do. std::variant uses an overloaded std::get method which throws an exception in the error case. We also do use nullptr in other places like container::vector::data() or memory::unique_ptr::get().

In this particular case, in my opinion it's not the task of the variant to check the validity but the task of the user. In this case the user is the expected and this class has the burden to check if the underlying variant can safely be accessed. By using Expects the error handler is then called in the expected.

Let's say the following would be defined behaviour, which error would returned from get_error?

auto sut = expected<Foo, Bar>::create_value(VALID_VALUE);
auto foo = sut.get_error();

[elfenpiff]

This statement is wrong, there is currently no undefined behavior in the expected since the variant returns a nullptr in those cases which is well defined.
But an expect call before hand makes sense so that we can forward this to some kind of error handling.

[mossmaurice]

@elfenpiff I don't fully get what you mean. The following is definitely UB and avoided with this PR:

auto sut = expected<Foo, Bar>::create_error(Bar::VALID_ERROR);
auto myValue = sut->memberVariableOfFoo;

Because as you wrote the ìox::variant will return a nullptr which cause UB in operator->.

[elfenpiff]

Isn't this wrong in detail. I would rather state that the error handler will be called and then it does what an error handler does. Please adjust this here and in the other descriptions.

[mossmaurice]

We currently use this wording in most of the classes e.g. iox::optional, iox::vector, so I followed along for consistency. But I know what you mean, hence adjusted the wording. The doxygen of the other classes should be adapted with #1032.