iceoryx icon indicating copy to clipboard operation
iceoryx copied to clipboard

WIP Iox #1394 fix axivion violation for optional hpp inl

Open saif-at-github opened this issue 1 year ago • 1 comments

Pre-Review Checklist for the PR Author

  1. [x] Code follows the coding style of CONTRIBUTING.md
  2. [x] Tests follow the best practice for testing
  3. [x] Changelog updated in the unreleased section including API breaking changes
  4. [x] Branch follows the naming format (iox-123-this-is-a-branch)
  5. [x] Commits messages are according to this guideline
    • [x] Commit messages have the issue ID (iox-#123 commit text)
    • [x] Commit messages are signed (git commit -s)
    • [ x Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  6. [x] Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. [x] Relevant issues are linked
  8. [x] Add sensible notes for the reviewer
  9. [x] All checks have passed (except task-list-completed)
  10. [x] All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  11. [x] 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
  • [ ] 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 TBD

saif-at-github avatar Sep 06 '22 13:09 saif-at-github

Codecov Report

Merging #1607 (34fb486) into master (e1e55dd) will increase coverage by 0.01%. The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1607      +/-   ##
==========================================
+ Coverage   77.34%   77.36%   +0.01%     
==========================================
  Files         366      366              
  Lines       14230    14236       +6     
  Branches     1991     1993       +2     
==========================================
+ Hits        11006    11013       +7     
  Misses       2591     2591              
+ Partials      633      632       -1     
Flag Coverage Δ
unittests 77.01% <94.11%> (+0.01%) :arrow_up:
unittests_timing 15.67% <17.64%> (+<0.01%) :arrow_up:

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

Impacted Files Coverage Δ
...fs/include/iceoryx_hoofs/internal/cxx/optional.inl 95.32% <94.11%> (+0.27%) :arrow_up:
iceoryx_hoofs/source/concurrent/loffli.cpp 82.85% <0.00%> (+2.85%) :arrow_up:

codecov[bot] avatar Sep 06 '22 14:09 codecov[bot]

@mossmaurice @FerdinandSpitzschnueffler I think all the comments are fixed. ready to merge.

saif-at-github avatar Oct 07 '22 11:10 saif-at-github

We should still test the new operator== and operator!= overloads where the first argument has type nullopt_t.

This is addressed.

saif-at-github avatar Oct 07 '22 11:10 saif-at-github

The public API of the optional comparision operators has changed. Please add this to doc/website/release-notes/iceoryx-unreleased.md as refactoring item and an additional migration path for users at the end of the file. Here I would say that on example with a comparison operator suffices.

Since the usage has not changed I think we don't necessarily need to mention this in the release note. @elfenpiff What do you think?

The public API of the optional comparision operators has changed. Please add this to doc/website/release-notes/iceoryx-unreleased.md as refactoring item and an additional migration path for users at the end of the file. Here I would say that on example with a comparison operator suffices.

Since the usage has not changed I think we don't necessarily need to mention this in the release note. @elfenpiff What do you think?

I am alright with this!

elfenpiff avatar Oct 28 '22 10:10 elfenpiff

The public API of the optional comparision operators has changed. Please add this to doc/website/release-notes/iceoryx-unreleased.md as refactoring item and an additional migration path for users at the end of the file. Here I would say that on example with a comparison operator suffices.

Since the usage has not changed we don't need to mention this in the release note. Thanks.

saif-at-github avatar Oct 28 '22 11:10 saif-at-github