iox-#1196 fix compiler warnings in hoofs tests #1453

elfenpiff · 2022-07-06T10:15:21Z

Pre-Review Checklist for the PR Author

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
- 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

Part of Enable clang-tidy checks and fix warnings in code #1196

…ts and correct first part of cxx tests Signed-off-by: Christian Eltzschig <[email protected]>

Signed-off-by: Christian Eltzschig <[email protected]>

mossmaurice

LGTM, left a few nitpicks.

iceoryx_hoofs/test/moduletests/test_cxx_forward_list.cpp

iceoryx_hoofs/test/moduletests/test_cxx_function_ref.cpp

iceoryx_hoofs/test/moduletests/test_cxx_list.cpp

iceoryx_hoofs/test/moduletests/test_cxx_stack.cpp

iceoryx_hoofs/test/moduletests/test_objectpool.cpp

iceoryx_hoofs/test/moduletests/test_relative_pointer.cpp

iceoryx_hoofs/test/moduletests/test_unit_duration.cpp

MatthiasKillat

Most comments refer to issues with introducing unnecessary casts (as far as I can see) when it instead would be better to change the signedness of some iteration variable or similar.

I think the goal should be to avoid these kinds of casts as they are usually a (interface) design problem.

MatthiasKillat · 2022-07-06T11:41:50Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/graphs/directed_graph.hpp

    using AdjacencyList = iox::cxx::vector<VertexType*, DEGREE_LIMIT>;

-    static constexpr Index_t INVALID_INDEX = -1;
+    static constexpr Index_t INVALID_INDEX = std::numeric_limits<uint64_t>::max();


This has logical implications if set to this maximum and should not be changed like this (as any positive integer is a valid value). I think it is better to keep the value signed. Furthermore we will never have more nodes than we can store with 32 bits (we would be out of memory first) and hence not use a 64 bit integer.

When suddenly the invalid value is positive I cannot guarantee it breaks something at the limits (which are not tested, as is the case for many classes...).

I know the graph is not used (leftover from some internals in the past) and if we will ever reuse something like this I will refactor it for more generality and performance.

I will reduce the size to 32 but would stick with unsigned since this type is used in array operator[] and this requires an unsigned integer. So I have to write a little less static_cast

MatthiasKillat · 2022-07-06T11:44:38Z

iceoryx_hoofs/test/integrationtests/test_lockfree_queue_stresstest.cpp

@@ -35,14 +35,14 @@ namespace
 {
 struct Data
 {
-    Data(int id = 0, size_t count = 0)
+    Data(uint64_t id = 0U, uint64_t count = 0)


Can be done of course but I thought in test code int is ok? Same with size_t, esecially if we interact with the STL (not the case here).

We have to be careful with upgrading everything to uint64_t without considering context as this may cause other issues. Here it is ok.

But with int and size_t we get warnings when we compare or assign it to uint64_t types which are returned by capacity, size or other functions.

iceoryx_hoofs/test/integrationtests/test_resizeable_lockfree_queue_stresstest.cpp

iceoryx_hoofs/test/moduletests/test_cxx_algorithm.cpp

MatthiasKillat · 2022-07-06T12:16:59Z

iceoryx_hoofs/test/moduletests/test_cxx_forward_list.cpp

@@ -313,26 +313,26 @@ TEST_F(forward_list_test, NotFullWhenFilledWithCapacityAndEraseOneElements)
 TEST_F(forward_list_test, NotFullWhenFilledWithCapacityAndEraseOneAndReinsertElements)
 {
    ::testing::Test::RecordProperty("TEST_ID", "7df1e41e-f3c2-4ea2-9ed1-ed68a5342ce2");
-    uint64_t i = 0U;
-    for (; i < sut.capacity(); ++i)
+    int64_t counter = 0;


I think this counter could be uint64_t and we would avoid all cast.

actually int64_t is better since sut requires signed integers and changing the sut to unsigned is even more work

Yes, less changes that way. In theory the tests can be redesigned but it is not worth it.

iceoryx_hoofs/test/moduletests/test_cxx_forward_list.cpp

MatthiasKillat · 2022-07-06T12:24:38Z

iceoryx_hoofs/test/moduletests/test_cxx_list.cpp

@@ -294,20 +294,20 @@ TEST_F(list_test, NotFullWhenFilledWithCapacityAndEraseOneElements)
 TEST_F(list_test, NotFullWhenFilledWithCapacityAndEraseOneAndReinsertElements)
 {
    ::testing::Test::RecordProperty("TEST_ID", "c7e70ac7-e476-43aa-976c-1ac78513c869");
-    uint64_t i = 0U;
-    for (; i < sut.capacity(); ++i)
+    int64_t counter = 0U;


I think counter should be unsigned to avoid the casts.

Has multiple occurrences in the following tests.

Same again, the list requires signed integers and switching to unsigned requires even more work.

iceoryx_hoofs/test/moduletests/test_cxx_stack.cpp

…make STACK_SIZE in test uint64_t again Signed-off-by: Christian Eltzschig <[email protected]>

Signed-off-by: Christian Eltzschig <[email protected]>

mossmaurice · 2022-07-06T13:28:53Z

@elfenpiff Ubuntu is still failing:

[ 27%] Building CXX object posh/CMakeFiles/iceoryx_posh_roudi.dir/source/roudi/memory/memory_block.cpp.o
/home/runner/work/iceoryx/iceoryx/iceoryx_hoofs/test/moduletests/test_concurrent_loffli.cpp:33:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]
   33 | #pragma GCC diagnostic ignored "-Wgnu-zero-variadic-macro-arguments"
      |

MatthiasKillat

Only a comment out of sync but should be fixed.

iceoryx_hoofs/test/integrationtests/test_resizeable_lockfree_queue_stresstest.cpp

MatthiasKillat · 2022-07-06T13:41:12Z

iceoryx_hoofs/test/moduletests/test_cxx_forward_list.cpp

@@ -313,26 +313,26 @@ TEST_F(forward_list_test, NotFullWhenFilledWithCapacityAndEraseOneElements)
 TEST_F(forward_list_test, NotFullWhenFilledWithCapacityAndEraseOneAndReinsertElements)
 {
    ::testing::Test::RecordProperty("TEST_ID", "7df1e41e-f3c2-4ea2-9ed1-ed68a5342ce2");
-    uint64_t i = 0U;
-    for (; i < sut.capacity(); ++i)
+    int64_t counter = 0;


Yes, less changes that way. In theory the tests can be redesigned but it is not worth it.

Signed-off-by: Christian Eltzschig <[email protected]>

codecov · 2022-07-06T17:01:26Z

Codecov Report

Merging #1453 (6dd45cc) into master (1e1ac61) will increase coverage by 0.02%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #1453      +/-   ##
==========================================
+ Coverage   78.75%   78.78%   +0.02%     
==========================================
  Files         377      377              
  Lines       14490    14490              
  Branches     2011     2011              
==========================================
+ Hits        11412    11416       +4     
+ Misses       2438     2437       -1     
+ Partials      640      637       -3

Flag	Coverage Δ
unittests	`78.44% <40.00%> (+0.02%)`	⬆️
unittests_timing	`14.87% <0.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...e/iceoryx_hoofs/internal/graphs/directed_graph.hpp	`92.20% <25.00%> (+1.29%)`	⬆️
...oofs/include/iceoryx_hoofs/internal/cxx/string.inl	`98.49% <100.00%> (ø)`
iceoryx_posh/source/roudi/port_manager.cpp	`84.42% <0.00%> (+0.18%)`	⬆️
...x_hoofs/internal/graphs/directed_acyclic_graph.hpp	`87.17% <0.00%> (+2.56%)`	⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp	`88.57% <0.00%> (+2.85%)`	⬆️

elfenpiff added 3 commits July 6, 2022 10:34

iox-eclipse-iceoryx#1196 Introduce warnings in CMakeLists.txt for tes…

e6edb13

…ts and correct first part of cxx tests Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1196 Suppress further compiler warnings in hoofs

3a7d7be

Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1196 Fix warning in further hoofs tests

6d4f09b

Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff requested review from mossmaurice and MatthiasKillat July 6, 2022 10:15

elfenpiff self-assigned this Jul 6, 2022

elfenpiff changed the title ~~Iox #1196 fix compiler warnings in hoofs tests~~ iox-#1196 fix compiler warnings in hoofs tests Jul 6, 2022

mossmaurice reviewed Jul 6, 2022

View reviewed changes

MatthiasKillat reviewed Jul 6, 2022

View reviewed changes

elfenpiff added 2 commits July 6, 2022 14:52

iox-eclipse-iceoryx#1196 Fix leftovers with missing U, capital F and …

6bc6ec7

…make STACK_SIZE in test uint64_t again Signed-off-by: Christian Eltzschig <[email protected]>

iox-eclipse-iceoryx#1196 Refine some integer types (signed/unsigned)

068e8f7

Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff requested review from mossmaurice and MatthiasKillat July 6, 2022 13:19

mossmaurice previously approved these changes Jul 6, 2022

View reviewed changes

elfenpiff dismissed mossmaurice’s stale review via ac9efd2 July 6, 2022 13:43

MatthiasKillat reviewed Jul 6, 2022

View reviewed changes

iox-eclipse-iceoryx#1196 Fix gcc warnings

b29fd03

Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff force-pushed the iox-#1196-fix-compiler-warnings-in-hoofs-tests branch from ac9efd2 to b29fd03 Compare July 6, 2022 13:46

elfenpiff requested review from MatthiasKillat and mossmaurice July 6, 2022 13:46

mossmaurice previously approved these changes Jul 6, 2022

View reviewed changes

elfenpiff dismissed mossmaurice’s stale review via 399bc62 July 6, 2022 13:58

elfenpiff force-pushed the iox-#1196-fix-compiler-warnings-in-hoofs-tests branch from 399bc62 to 2802cf8 Compare July 6, 2022 14:55

iox-eclipse-iceoryx#1196 Remove superfluous ;

07359d8

Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff force-pushed the iox-#1196-fix-compiler-warnings-in-hoofs-tests branch from 2802cf8 to 07359d8 Compare July 6, 2022 15:24

iox-eclipse-iceoryx#1196 Fix narrowing warning

899e757

Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff force-pushed the iox-#1196-fix-compiler-warnings-in-hoofs-tests branch from d41eb82 to 971d88e Compare July 6, 2022 16:12

iox-eclipse-iceoryx#1196 Remove superfluous ;

6dd45cc

Signed-off-by: Christian Eltzschig <[email protected]>

elfenpiff force-pushed the iox-#1196-fix-compiler-warnings-in-hoofs-tests branch from 971d88e to 6dd45cc Compare July 6, 2022 16:33

MatthiasKillat approved these changes Jul 6, 2022

View reviewed changes

FerdinandSpitzschnueffler approved these changes Jul 6, 2022

View reviewed changes

elfenpiff merged commit 114afea into eclipse-iceoryx:master Jul 6, 2022

elfenpiff deleted the iox-#1196-fix-compiler-warnings-in-hoofs-tests branch July 6, 2022 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#1196 fix compiler warnings in hoofs tests #1453

iox-#1196 fix compiler warnings in hoofs tests #1453

elfenpiff commented Jul 6, 2022 •

edited

Loading

mossmaurice left a comment

MatthiasKillat left a comment

MatthiasKillat Jul 6, 2022

elfenpiff Jul 6, 2022

MatthiasKillat Jul 6, 2022

elfenpiff Jul 6, 2022

MatthiasKillat Jul 6, 2022

elfenpiff Jul 6, 2022

MatthiasKillat Jul 6, 2022

MatthiasKillat Jul 6, 2022

elfenpiff Jul 6, 2022

mossmaurice commented Jul 6, 2022

MatthiasKillat left a comment

MatthiasKillat Jul 6, 2022

codecov bot commented Jul 6, 2022

iox-#1196 fix compiler warnings in hoofs tests #1453

iox-#1196 fix compiler warnings in hoofs tests #1453

Conversation

elfenpiff commented Jul 6, 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

mossmaurice left a comment

Choose a reason for hiding this comment

MatthiasKillat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice commented Jul 6, 2022

MatthiasKillat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2022

Codecov Report

elfenpiff commented Jul 6, 2022 •

edited

Loading