Compilation warnings with g++ 9.1.1 #16992

I just compiled with a new g++ version for the first time, and noticed some warnings. I don't know how serious these are. I don't think they're very bad (concerning deprecations mostly):

• "specified bound 12 equals destination size"

In file included from /usr/include/string.h:494,
                 from ./serialize.h:20,
                 from ./netaddress.h:13,
                 from ./protocol.h:13,
                 from protocol.cpp:6:
In function ‘char* strncpy(char*, const char*, size_t)’,
    inlined from ‘CMessageHeader::CMessageHeader(const unsigned char (&)[4], const char*, unsigned int)’ at protocol.cpp:91:12:
/usr/include/bits/string_fortified.h:106:34: warning: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ specified bound 12 equals destination size [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

• implicitly-declared ‘PartiallySignedTransaction& PartiallySignedTransaction::operator=(const PartiallySignedTransaction&)’ is deprecated [-Wdeprecated-copy]

psbt.cpp: In function ‘TransactionError CombinePSBTs(PartiallySignedTransaction&, const std::vector<PartiallySignedTransaction>&)’:
psbt.cpp:335:19: warning: implicitly-declared ‘PartiallySignedTransaction& PartiallySignedTransaction::operator=(const PartiallySignedTransaction&)’ is deprecated [-Wdeprecated-copy]
  335 |     out = psbtxs[0]; // Copy the first one
      |                   ^
In file included from psbt.cpp:5:
./psbt.h:398:5: note: because ‘PartiallySignedTransaction’ has user-provided ‘PartiallySignedTransaction::PartiallySignedTransaction(const PartiallySignedTransaction&)’
  398 |     PartiallySignedTransaction(const PartiallySignedTransaction& psbt_in) : tx(psbt_in.tx), inputs(psbt_in.inputs), outputs(psbt_in.outputs), unknown(psbt_in.unknown) {}
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~

• warning: redundant move in return statement

interfaces/chain.cpp: In member function ‘virtual std::unique_ptr<interfaces::Chain::Lock> interfaces::{anonymous}::ChainImpl::lock(bool)’:
interfaces/chain.cpp:249:25: warning: redundant move in return statement [-Wredundant-move]
  249 |         return std::move(result);  
      |                ~~~~~~~~~^~~~~~~~                   
interfaces/chain.cpp:249:25: note: remove ‘std::move’ call

Also: #15822

interfaces/chain.cpp:249:25: warning: redundant move in return statement [-Wredundant-move]
  249 |         return std::move(result);  
      |                ~~~~~~~~~^~~~~~~~                   
interfaces/chain.cpp:249:25: note: remove ‘std::move’ call

std::move here was needed to support older versions of clang, I think including the version used for travis macos builds (https://stackoverflow.com/questions/36752678/clang-returning-stdunique-ptr-with-type-conversion)

Could maybe use some macro magic to avoid both the clang error without std::move and the gcc warning with std::move if the clang bug is still a concern.

@ryanofsky Thanks for the info! Will try to figure out if this is still necessary, and if so, if we can find an alternative work-around that doesn't make gcc complain.

Edit: it is still necessary. And not just for clang. Without the std::move it fails on trusty, xenial and MacOSX.

Libreoffice stumbled on the same problem: https://cgit.freedesktop.org/libreoffice/core/commit/?id=dc06c8f4989fc28d0c31ebd333e53dfe0e0f5f66

What they did was detect it in the build system:

HAVE_CXX_CWG1579_FIX=
AC_MSG_CHECKING([whether $CXX has a fix for CWG1579])
AC_LANG_PUSH([C++])
save_CXXFLAGS=$CXXFLAGS
CXXFLAGS="$CXXFLAGS $CXXFLAGS_CXX11"
AC_COMPILE_IFELSE([AC_LANG_SOURCE([
        #include <memory>
        struct S1 {};
        struct S2: S1 {};
        std::unique_ptr<S1> f() {
            std::unique_ptr<S2> s2(new S2);
            return s2;
        }
    ])], [
        AC_DEFINE([HAVE_CXX_CWG1579_FIX],[1])
        AC_MSG_RESULT([yes])
    ], [AC_MSG_RESULT([no])])
CXXFLAGS=$save_CXXFLAGS
AC_LANG_POP([C++])
AC_SUBST([HAVE_CXX_CWG1579_FIX])```

Then use this to determine whether to use std::move for the upcast

#if HAVE_CXX_CWG1579_FIX
            return pNew;
#else
            return std::move(pNew);
#endif

… not sure if this is worth it for a warning, but FWIW

http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579

Not worth fixing, it bounces from one C++ compiler issue to another (see discussion in #16995).