iox-#1196 Fix left-over clang-tidy warnings in `cxx::vector` and remove space in suppressions #1525

mossmaurice · 2022-07-19T08:19:00Z

Signed-off-by: Simon Hoinkis [email protected]

Pre-Review Checklist for the PR Author

Notes for Reviewer

Supress some left-over clang-tidy findings
// NOLINTNEXTLINE (foobar) suppresses all findings
- Added a check to ensure // NOLINTNEXTLINE(foobar)

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
- Each unit test case has a unique UUID
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

All open points are addressed and tracked via issues

References

Closes Enable clang-tidy checks and fix warnings in code #1196 (partly)

dkroenke · 2022-07-19T09:29:22Z

Can we consider to add these files to https://github.com/eclipse-iceoryx/iceoryx/blob/master/clang-tidy-files-to-scan.txt?
Is it intended to close the ticket once the PR is merged or do we want to keep it open? If the latter case applies you can change it to Relates.

mossmaurice · 2022-07-19T09:39:06Z

@dkroenke

Can we consider to add these files to https://github.com/eclipse-iceoryx/iceoryx/blob/master/clang-tidy-files-to-scan.txt? Is it intended to close the ticket once the PR is merged or do we want to keep it open?

I don't understand the strategy here. Shall we add each class after having it ticked in the list? Or will we enable everything once done with #1196?

If the latter case applies you can change it to Relates.

Unfortunately, that doesn't work. PR and issue are then not linked.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl

elfenpiff · 2022-07-19T13:27:53Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl

    return const_cast<T&>(const_cast<const vector<T, Capacity>*>(this)->at_unchecked(index));
 }

 template <typename T, uint64_t Capacity>
 const T& vector<T, Capacity>::at_unchecked(const uint64_t index) const noexcept
 {
    // AXIVION Next Construct AutosarC++19_03-A5.2.4 : Type-safety ensured by template parameter
-    // NOLINTNEXTLINE (cppcoreguidelines-pro-type-reinterpret-cast)
+    // NOLINTJUSTIFICATION User accessible method at() performs bounds check


I think bound checks have nothing to do with a reinterpret_cast usage.

@elfenpiff You have to look more carefully above and below ;) Here, two warnings are suppressed:

reinterpret_cast for Axivion with justification and clang-tidy without duplicate justification

Bounds safety issue of working with raw array instead of std::span, justification only for clang-tidy

elfenpiff · 2022-07-19T13:28:40Z

iceoryx_hoofs/test/moduletests/test_cxx_function_ref.cpp

@@ -166,8 +166,11 @@ TEST_F(function_refDeathTest, CallMovedFromLeadsToTermination)
    function_ref<int()> sut1{lambda};
    function_ref<int()> sut2{std::move(sut1)};
    // Use after move is tested here
-    // NOLINTNEXTLINE (bugprone-use-after-move)
+    // NOLINTBEGIN(bugprone-use-after-move, hicpp-invalid-access-moved, cppcoreguidelines-pro-type-vararg,


Does multiline NOLINTBEGIN work? Is this ignored when you run clang-tidy manually?

@elfenpiff Unfortunately, I'm still running clang 13 locally, which does not support this syntax. However, the clang15 in the CI seems to run fine with.

Works also locally for me.

iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp

iceoryx_hoofs/test/moduletests/test_cxx_variant.cpp

iceoryx_hoofs/test/moduletests/test_cxx_unique_ptr.cpp

iceoryx_hoofs/test/moduletests/test_cxx_function_ref.cpp

tools/scripts/clang_tidy_check.sh

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/variant_internal.hpp

FerdinandSpitzschnueffler · 2022-07-19T17:34:17Z

iceoryx_hoofs/test/moduletests/test_cxx_function_ref.cpp

@@ -166,8 +166,11 @@ TEST_F(function_refDeathTest, CallMovedFromLeadsToTermination)
    function_ref<int()> sut1{lambda};
    function_ref<int()> sut2{std::move(sut1)};
    // Use after move is tested here
-    // NOLINTNEXTLINE (bugprone-use-after-move)
+    // NOLINTBEGIN(bugprone-use-after-move, hicpp-invalid-access-moved, cppcoreguidelines-pro-type-vararg,


Works also locally for me.

…ctor Signed-off-by: Simon Hoinkis <[email protected]>

…and add CI check Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

codecov · 2022-07-19T23:19:07Z

Codecov Report

Merging #1525 (3eb629b) into master (f85ee4b) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 3eb629b differs from pull request most recent head 7eb4a39. Consider uploading reports for the commit 7eb4a39 to get more accurate results

@@            Coverage Diff             @@
##           master    #1525      +/-   ##
==========================================
+ Coverage   78.96%   78.98%   +0.01%     
==========================================
  Files         378      378              
  Lines       14464    14464              
  Branches     2010     2010              
==========================================
+ Hits        11422    11424       +2     
+ Misses       2412     2411       -1     
+ Partials      630      629       -1

Flag	Coverage Δ
unittests	`78.64% <ø> (+0.01%)`	⬆️
unittests_timing	`14.90% <ø> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...ceoryx_hoofs/include/iceoryx_hoofs/cxx/variant.hpp	`100.00% <ø> (ø)`
iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp	`100.00% <ø> (ø)`
...nclude/iceoryx_hoofs/internal/cxx/function_ref.inl	`93.33% <ø> (ø)`
...lude/iceoryx_hoofs/internal/cxx/poor_mans_heap.inl	`100.00% <ø> (ø)`
.../include/iceoryx_hoofs/internal/cxx/unique_ptr.inl	`96.66% <ø> (ø)`
...ofs/include/iceoryx_hoofs/internal/cxx/variant.inl	`95.78% <ø> (ø)`
...de/iceoryx_hoofs/internal/cxx/variant_internal.hpp	`78.00% <ø> (ø)`
...oofs/include/iceoryx_hoofs/internal/cxx/vector.inl	`96.59% <ø> (ø)`
iceoryx_hoofs/source/concurrent/loffli.cpp	`91.42% <0.00%> (+5.71%)`	⬆️

mossmaurice added the clang-tidy Warning of a clang-tidy rule label Jul 19, 2022

mossmaurice requested review from dkroenke and FerdinandSpitzschnueffler July 19, 2022 08:19

mossmaurice self-assigned this Jul 19, 2022

mossmaurice force-pushed the iox-#1196-fix-more-clang-tidy-warnings-in-vector branch from aa36345 to 909afb4 Compare July 19, 2022 08:34

mossmaurice changed the title ~~iox-#1196 Fix left-over clang-tidy warnings in cxx::vector~~ iox-#1196 Fix left-over clang-tidy warnings in cxx::vector and remove space in suppressions Jul 19, 2022

mossmaurice force-pushed the iox-#1196-fix-more-clang-tidy-warnings-in-vector branch from e35a823 to 179f391 Compare July 19, 2022 10:04

elfenpiff reviewed Jul 19, 2022

View reviewed changes

FerdinandSpitzschnueffler reviewed Jul 19, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl Show resolved Hide resolved

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl Outdated Show resolved Hide resolved

mossmaurice removed the request for review from dkroenke July 19, 2022 13:52

FerdinandSpitzschnueffler previously approved these changes Jul 19, 2022

View reviewed changes

mossmaurice dismissed FerdinandSpitzschnueffler’s stale review via fcecd2d July 19, 2022 22:41

mossmaurice added 4 commits July 20, 2022 00:46

iox-eclipse-iceoryx#1196 Fix left-over clang-tidy warnings in cxx::ve…

aa80ea8

…ctor Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#1196 Remove space between NOLINTNEXTLINE and '(' …

2c99a38

…and add CI check Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#1196 Add missing 'NOLINTJUSTIFICATION'

1e48d5e

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#1196 Suppress return code in clang-tidy script

7eb4a39

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#1196-fix-more-clang-tidy-warnings-in-vector branch 2 times, most recently from 3eb629b to 7eb4a39 Compare July 19, 2022 23:03

mossmaurice requested review from elfenpiff and FerdinandSpitzschnueffler July 19, 2022 23:03

FerdinandSpitzschnueffler approved these changes Jul 20, 2022

View reviewed changes

elfenpiff approved these changes Jul 20, 2022

View reviewed changes

mossmaurice merged commit d550821 into eclipse-iceoryx:master Jul 20, 2022

mossmaurice deleted the iox-#1196-fix-more-clang-tidy-warnings-in-vector branch July 20, 2022 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#1196 Fix left-over clang-tidy warnings in `cxx::vector` and remove space in suppressions #1525

iox-#1196 Fix left-over clang-tidy warnings in `cxx::vector` and remove space in suppressions #1525

mossmaurice commented Jul 19, 2022 •

edited

Loading

dkroenke commented Jul 19, 2022

mossmaurice commented Jul 19, 2022 •

edited

Loading

elfenpiff Jul 19, 2022

mossmaurice Jul 19, 2022

elfenpiff Jul 19, 2022

mossmaurice Jul 19, 2022

FerdinandSpitzschnueffler Jul 19, 2022

FerdinandSpitzschnueffler Jul 19, 2022

codecov bot commented Jul 19, 2022 •

edited

Loading

iox-#1196 Fix left-over clang-tidy warnings in cxx::vector and remove space in suppressions #1525

iox-#1196 Fix left-over clang-tidy warnings in cxx::vector and remove space in suppressions #1525

Conversation

mossmaurice commented Jul 19, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

dkroenke commented Jul 19, 2022

mossmaurice commented Jul 19, 2022 • edited Loading

elfenpiff Jul 19, 2022

Choose a reason for hiding this comment

mossmaurice Jul 19, 2022

Choose a reason for hiding this comment

elfenpiff Jul 19, 2022

Choose a reason for hiding this comment

mossmaurice Jul 19, 2022

Choose a reason for hiding this comment

FerdinandSpitzschnueffler Jul 19, 2022

Choose a reason for hiding this comment

FerdinandSpitzschnueffler Jul 19, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2022 • edited Loading

Codecov Report

iox-#1196 Fix left-over clang-tidy warnings in `cxx::vector` and remove space in suppressions #1525

iox-#1196 Fix left-over clang-tidy warnings in `cxx::vector` and remove space in suppressions #1525

mossmaurice commented Jul 19, 2022 •

edited

Loading

mossmaurice commented Jul 19, 2022 •

edited

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading