test: fix sign comparison warning in socket tests #21159

fanquake · 2021-02-12T02:15:41Z

This fixes:

In file included from test/sock_tests.cpp:10:
In file included from /usr/local/include/boost/test/unit_test.hpp:18:
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare]
    return left == right;
           ~~~~ ^  ~~~~~

which was introduced in #20788.

This fixes: ```bash In file included from test/sock_tests.cpp:10: In file included from /usr/local/include/boost/test/unit_test.hpp:18: In file included from /usr/local/include/boost/test/test_tools.hpp:46: /usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare] return left == right; ~~~~ ^ ~~~~~ ``` which was introduced in bitcoin#20788.

vasild

ACK 9cc8e30

Which compiler is that? I guess we don't have a CI run with that compiler and -Werror? Otherwise it would have been spotted before merge. I don't get a warning with or without this patch with Clang 12.

vasild · 2021-02-12T08:25:25Z

src/test/sock_tests.cpp

@@ -95,7 +95,7 @@ static void CreateSocketPair(int s[2])
 static void SendAndRecvMessage(const Sock& sender, const Sock& receiver)
 {
    const char* msg = "abcd";
-    constexpr size_t msg_len = 4;
+    constexpr ssize_t msg_len = 4;


This is used also as 3rd argument to strncmp() which is of type size_t. I hope this change will not produce a warning for that. If it does, then a typecast would be necessary.

practicalswift · 2021-02-12T09:49:32Z

cr ACK 9cc8e30

Agree with @vasild regarding adding -Werror for this diagnostic if possible :)

Sjors · 2021-02-12T15:19:48Z

Odd, I'm pretty we did have a -Werror check on Travis, back in the day...

Anyway, it fixes the error for me on macOS 11.2 with Boost 1.75

vasild · 2021-02-12T15:35:09Z

@Sjors, (without looking) I am sure there are some -Werror runs on CI, but probably not with the compiler that gets upset by this code.

If you observe the warning, which compiler is that?

No warning for me with Clang 12.0.0 and Boost 1.72.

Sjors · 2021-02-12T16:08:33Z

Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin20.3.0

So perhaps the difference is in the boost version?

jonatack · 2021-02-12T16:29:01Z

I didn't see any warnings with Clang 9.0.1-16 and Boost 1.74.0.3 on Debian.

fanquake · 2021-02-17T00:25:52Z

It's unclear to me why the macOS 10.15 native CI run from #20788 didn't fail, as that should be the same setup of Apple Clang && boost-1.75.0_1 as where I'm seeing the warning. It was also being compiled with -Werror=sign-compare.

I don't think this is a Boost version difference, as I see the same warning whether I compile with Boost 1.71.0 out of depends:

In file included from test/sock_tests.cpp:10:
In file included from /Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/unit_test.hpp:18:
In file included from /Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/test_tools.hpp:46:
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare]
    return left == right;
           ~~~~ ^  ~~~~~
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<long, unsigned long>' requested here
        return equal_impl( left, right );
               ^
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<long, unsigned long>' requested here
        return call_impl( left, right, left_is_array() );
               ^
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<long, unsigned long>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
                                                 ^
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/preprocessor/repetition/repeat.hpp:29:26: note: expanded from macro 'BOOST_PP_REPEAT'
# define BOOST_PP_REPEAT BOOST_PP_CAT(BOOST_PP_REPEAT_, BOOST_PP_AUTO_REC(BOOST_PP_REPEAT_P, 4))
                         ^
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
#    define BOOST_PP_CAT(a, b) BOOST_PP_CAT_I(a, b)
                               ^
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
#    define BOOST_PP_CAT_I(a, b) a ## b
                                 ^
<scratch space>:22:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/sock_tests.cpp:101:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, long, unsigned long>' requested here
    BOOST_CHECK_EQUAL(sender.Send(msg, msg_len, 0), msg_len);
    ^
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/tools/old/interface.hpp:154:45: note: expanded from macro 'BOOST_CHECK_EQUAL'
#define BOOST_CHECK_EQUAL( L, R )           BOOST_TEST_TOOL_IMPL( 0, \
                                            ^
/Users/michael/github/fanquake-bitcoin/depends/x86_64-apple-darwin19.6.0/include/boost/test/tools/old/interface.hpp:67:47: note: expanded from macro 'BOOST_TEST_TOOL_IMPL'
    BOOST_PP_IF( frwd_type, report_assertion, check_frwd ) (                    \
                                              ^
1 warning generated.

or 1.75.0 from brew:

In file included from test/sock_tests.cpp:10:
In file included from /usr/local/include/boost/test/unit_test.hpp:18:
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare]
    return left == right;
           ~~~~ ^  ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<long, unsigned long>' requested here
        return equal_impl( left, right );
               ^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<long, unsigned long>' requested here
        return call_impl( left, right, left_is_array() );
               ^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<long, unsigned long>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
                                                 ^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
# define BOOST_PP_REPEAT BOOST_PP_CAT(BOOST_PP_REPEAT_, BOOST_PP_AUTO_REC(BOOST_PP_REPEAT_P, 4))
                         ^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
#    define BOOST_PP_CAT(a, b) BOOST_PP_CAT_I(a, b)
                               ^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CA  CXX      test/test_bitcoin-util_threadnames_tests.o
T_I'
#    define BOOST_PP_CAT_I(a, b) a ## b
                                 ^
<scratch space>:154:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/sock_tests.cpp:101:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, long, unsigned long>' requested here
    BOOST_CHECK_EQUAL(sender.Send(msg, msg_len, 0), msg_len);
    ^
/usr/local/include/boost/test/tools/old/interface.hpp:154:45: note: expanded from macro 'BOOST_CHECK_EQUAL'
#define BOOST_CHECK_EQUAL( L, R )           BOOST_TEST_TOOL_IMPL( 0, \
                                            ^
/usr/local/include/boost/test/tools/old/interface.hpp:67:47: note: expanded from macro 'BOOST_TEST_TOOL_IMPL'
    BOOST_PP_IF( frwd_type, report_assertion, check_frwd ) (                    \
                                              ^
1 warning generated.

9cc8e30 test: fix sign comparison warning in socket tests (fanquake) Pull request description: This fixes: ```bash In file included from test/sock_tests.cpp:10: In file included from /usr/local/include/boost/test/unit_test.hpp:18: In file included from /usr/local/include/boost/test/test_tools.hpp:46: /usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare] return left == right; ~~~~ ^ ~~~~~ ``` which was introduced in bitcoin#20788. ACKs for top commit: practicalswift: cr ACK 9cc8e30 vasild: ACK 9cc8e30 Tree-SHA512: 7069a4fde5cec01be03f8477fe396e53658f170efbf1d9ef3339d553bb90a2be9f4acd6b348127b14cd2f91426e0cd1fc35d2d3c9f201cf748c0cf50f47e46a5

maflcko · 2021-02-17T09:26:11Z

See #19123

fanquake added the Tests label Feb 12, 2021

fanquake requested a review from vasild February 12, 2021 02:15

vasild approved these changes Feb 12, 2021

View reviewed changes

fanquake merged commit 7c8e605 into bitcoin:master Feb 17, 2021

fanquake deleted the sign_compare_sock_tests branch February 17, 2021 00:27

maflcko mentioned this pull request Feb 18, 2021

test: Avoid comparision of integers with different signs #21228

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: fix sign comparison warning in socket tests #21159

test: fix sign comparison warning in socket tests #21159

Uh oh!

fanquake commented Feb 12, 2021

Uh oh!

vasild left a comment

Uh oh!

vasild Feb 12, 2021

Uh oh!

practicalswift commented Feb 12, 2021

Uh oh!

Sjors commented Feb 12, 2021 •

edited

Loading

Uh oh!

vasild commented Feb 12, 2021

Uh oh!

Sjors commented Feb 12, 2021 •

edited

Loading

Uh oh!

jonatack commented Feb 12, 2021

Uh oh!

fanquake commented Feb 17, 2021

Uh oh!

maflcko commented Feb 17, 2021

Uh oh!

Uh oh!

test: fix sign comparison warning in socket tests #21159

test: fix sign comparison warning in socket tests #21159

Uh oh!

Conversation

fanquake commented Feb 12, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 12, 2021

Uh oh!

Sjors commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Feb 12, 2021

Uh oh!

Sjors commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Feb 12, 2021

Uh oh!

fanquake commented Feb 17, 2021

Uh oh!

maflcko commented Feb 17, 2021

Uh oh!

Uh oh!

Sjors commented Feb 12, 2021 •

edited

Loading

Sjors commented Feb 12, 2021 •

edited

Loading