Fix icc (Intel C++ compiler) warnings #14706

Rationale:

• By silencing these warnings real issues become easier to spot when building with icc.

Fix icc (Intel C++ compiler) warnings:

protocol.cpp(83): warning [#68](/bitcoin-bitcoin/68/): integer conversion resulted in a change of sign
      nMessageSize = -1;
                     ^
test/serialize_tests.cpp(203): warning [#68](/bitcoin-bitcoin/68/): integer conversion resulted in a change of sign
          uint64_t j = -1;
                       ^
./validationinterface.h(150): warning [#1098](/bitcoin-bitcoin/1098/): the qualifier on this friend declaration is ignored
      friend void ::RegisterValidationInterface(CValidationInterface*);
                  ^
./validationinterface.h(151): warning [#1098](/bitcoin-bitcoin/1098/): the qualifier on this friend declaration is ignored
      friend void ::UnregisterValidationInterface(CValidationInterface*);
                  ^
./validationinterface.h(152): warning [#1098](/bitcoin-bitcoin/1098/): the qualifier on this friend declaration is ignored
      friend void ::UnregisterAllValidationInterfaces();
                  ^
./validationinterface.h(160): warning [#1098](/bitcoin-bitcoin/1098/): the qualifier on this friend declaration is ignored
      friend void ::RegisterValidationInterface(CValidationInterface*);
                  ^
./validationinterface.h(161): warning [#1098](/bitcoin-bitcoin/1098/): the qualifier on this friend declaration is ignored
      friend void ::UnregisterValidationInterface(CValidationInterface*);
                  ^
./validationinterface.h(162): warning [#1098](/bitcoin-bitcoin/1098/): the qualifier on this friend declaration is ignored
      friend void ::UnregisterAllValidationInterfaces();
                  ^
./validationinterface.h(163): warning [#1098](/bitcoin-bitcoin/1098/): the qualifier on this friend declaration is ignored
      friend void ::CallFunctionInValidationInterfaceQueue(std::function<void ()> func);
                  ^

nit: how about including the warnings in the commit message, so the changes are explained in the repo itself?

@Empact Good idea! Fixed :-)

NAK. Consensus changes without a proof of no effective change and without any justification given (beyond the implicit 'silences a warning in an unsupported compiler'), and also fixing type warnings via peppering in casts (which at best merely silences the warning but doesn't change the behaviour it was warning about).

@gmaxwell

Thanks for the quick review!

I've now removed this change to interpreter.cpp (avoidance of unintended unsigned integer wraparound):

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 95b25b491..3e77d6b9c 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -51,8 +51,8 @@ bool CastToBool(const valtype& vch)
  * Script is a stack machine (like Forth) that evaluates a predicate
  * returning a bool indicating valid or not.  There are no loops.
  */
-#define stacktop(i)  (stack.at(stack.size()+(i)))
-#define altstacktop(i)  (altstack.at(altstack.size()+(i)))
+#define stacktop(i)  (stack.at((size_t)((int)stack.size() + (int)(i))))
+#define altstacktop(i)  (altstack.at((size_t)((int)altstack.size() + (int)(i))))
 static inline void popstack(std::vector<valtype>& stack)
 {
     if (stack.empty())

I've also removed the explicit casts you mentioned (in streams_tests.cpp). My intention with this change was to make the current implicit conversion explicit and thereby silencing the warning:

--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -175,7 +175,7 @@ BOOST_AUTO_TEST_CASE(streams_serializedata_xor)
     ds.insert(ds.begin(), in.begin(), in.end());
     key.clear();

-    key.push_back('\xff');
+    key.push_back((unsigned char)'\xff');
     ds.Xor(key);
     BOOST_CHECK_EQUAL(
             std::string(expected_xor.begin(), expected_xor.end()),
@@ -194,7 +194,7 @@ BOOST_AUTO_TEST_CASE(streams_serializedata_xor)
     ds.insert(ds.begin(), in.begin(), in.end());

     key.clear();
-    key.push_back('\xff');
+    key.push_back((unsigned char)'\xff');
     key.push_back('\x0f');

     ds.Xor(key);

I've also added the rationale to the description.

Please re-review :-)

(Some context: I'm setting up a continuous icc build and I thought it would be nice to get rid of some noise. If that is a goal that is not shared by my fellow contributors who are potentially also building with icc from time to time I'll keep these changes local. Let me know and I'll happily close!)

[…] in an unsupported compiler

Based on the comment #12695 (comment) I thought we didn't make any assumptions about what compilers will be used:

Generally we don't want to make any assumptions about what compiler will be used, unless an option is known to directly conflict between common ones. I don't think that's the case here. The code also builds fine with msvc, and I'd hope that icc works as well.

Should we document the set of officially "supported" compilers (gcc + clang?) if such a thing exists? :-)

@practicalswift Release binaries are built using gcc (linux and windows) and clang (macos), nothing else. We aim to have things work in other compilers for those who self-compile without the use of the depends system, but those obviously don't carry the same amount of support or testing.

@sipa Thanks for clarifying! Makes perfect sense.