-
Notifications
You must be signed in to change notification settings - Fork 717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug] Compiler warnings #2076
Comments
Thanks for reporting these @kyeno . |
Hey, thank you @random-zebra! |
… in CreateSig 2523f81 [Refactor] Pass caught logic_error by reference in CreateSig (random-zebra) Pull request description: Exceptions/errors caught should be passed by reference. If the catch-block is not modifying the exception/error, the reference should be `const` (same as we did way back in #979) Fix one instance in `TransactionSignatureCreator::CreateSig` where we are passing a logic_error by copy, causing the `catching polymorphic type` warning reported in #2076 ACKs for top commit: furszy: utACK 2523f81 Fuzzbawls: utACK 2523f81 Tree-SHA512: d13a0a2a9372610f3a5e67016053afbbac94d30f5d3dd8568352f7fd99356336058405f8526eaf1e6f751e2e48d7405425ad7a8a3a3f0ba8c6db620d12710ad2
Hey! The environment and compilation params were identical as in the first case. It's less than it was, but still throws quite a bit of warnings:
|
@kyeno the pull requests above fix (most of) these warnings. |
@random-zebra oh, I didn't notice they were not merged; I just ran for master compile. Still, probably some new findings there too. :) |
ae41ebe Remove redundant explicitly defined copy ctors (Dan Raviv) 454aa37 Trivial: explicit (default) copy-constructor for CTransaction (random-zebra) 2af2306 Cleanup: Remove redundant copy-constructor for CSignedMessage (random-zebra) e28259d Masternode: proper copy-assignment for CMasternode/CmasternodePing (random-zebra) Pull request description: Fix the `-Wdeprecated-copy` warnings listed in #2076 **Background** While perfectly valid in C++98, implicit generation of the copy constructor is declared as deprecated in C++11, when there is already a user-defined copy assignment operator, or vice-versa. The rationale behind this is the "rule of three" (https://en.cppreference.com/w/cpp/language/rule_of_three) > If a class requires a user-defined destructor, a user-defined copy constructor, or a user-defined copy assignment operator, it almost certainly requires all three. **Issue - Fix** We have several places that define redundant copy assignment operators (and then use the implicit copy constructor), or that do need non-default ctors/dtors but don't define copy assignment. (See also bitcoin#11161, which is also cherry-picked here) Specifically: - `CSignedMessage`/`CMasternodePing`/`CMasternode` use a weird copy-assignment that *swaps* with the argument (which, of course, cannot be "const" then). Here we remove all ::swap implementations, and provide actual copy assignment operators. - `CTransaction` has an explicit copy-assignment operator, but uses the implicit copy-ctor in `CMerkleTx` constructor. - `CFeeRate` and `CTxMemPoolEntry` have explicitly defined copy ctor, which can (and should, for the rule-of-three) be replaced by the default one (bitcoin#11161). **Note** Reason why these warnings pop up just now. Since Clang 10.0 and GCC 9.3, `-Wextra` enables `-Wdeprecated-copy` by default, and that is very spammy, especially when compiling with the QT GUI (bitcoin#15822 / bitcoin#18419). It is so spammy (also with `boost-1.72.0`) that Bitcoin decided to temporarily suppress this warning (bitcoin#18738) until QT fixes it upstream, and boost is definitely removed in 0.22 (bitcoin#18967) ACKs for top commit: furszy: utACK ae41ebe Tree-SHA512: 1f59f9f7426abbf4b7d4bb0d1ac50491eb52f776e2a018de3acc8ab381af3b69db6b9a5b60c04b48f0d495e560eaa1e3d19be03d133f230daca66ba66e766289
…eader c223041 net: Fail instead of truncate command name in CMessageHeader (Wladimir J. van der Laan) Pull request description: Fix the `-Wstringop-truncation` warning reported in #2076, cherry-picking the first commit of bitcoin#16995 > Replace the memset/strncpy dance in `CMessageHeader::CMessageHeader` with explicit code that copies then name and asserts the length. > > This removes a warning in g++ 9.1.1 and IMO makes the code more readable by not relying on strncpy padding and silent truncation behavior. ACKs for top commit: furszy: utACK c223041 Fuzzbawls: utACK c223041 Tree-SHA512: 1c1fecc990f4d19fa5a1eb559132e81ee904af5557e3a63256d08741e201996de1f116267025317cf0c236872d121af958798a4de9bbc9a5afbcd601b768a9aa
… opaque blobs dea250c [Core] Migrate uint160 (CKeyID/CScriptID) to opaque blobs (random-zebra) 86a106e [Tests] Use arith_uint160 instead of uint160 in uint256_tests (random-zebra) 12240dc [BUG] fix a few places improperly checking boost::get<CKeyID> (random-zebra) Pull request description: Keep going with the migration started in PIVX-Project#1395 and PIVX-Project#1414. This time migrate the whole class `uint160`, so it is defined as a child of `base_blob` (essentially replace `uint160` with `blob_uint160`), thus migrating `CKeyID` and `CScriptID` as well. Since opaque blobs (unlike the `arith_uint` classes) have trivial copy-assignment operator, this fixes the "scary" `-Wclass-memaccess` warning reported in PIVX-Project#2076 (tested before/after patch, with gcc v9.3.0) Bonus: this fixes also a minor bug found along the way: few places where we are checking the result of `boost::get<CKeyID>` over the pointed `CKeyID`, which shouldn't even have a `bool operator()`, and might even be un-initialized (though we always verify `IsValidDestination` before, so this is mostly redundant) instead of checking the pointer itself. ACKs for top commit: furszy: looking good, utACK dea250c Fuzzbawls: utACK dea250c Tree-SHA512: 2e6d6e2adfcd8a1c32941f0480b882ea801fe1816eaa0c5050efc506c0aa09f6d589d53fb13798a17ffd2858021c3a0ccd50465acbc7d5c5a71dc0fb0f398756
Guess we can close this now, if there isn't anything major left. |
PIVX master branch
commit 8d4f649
Machine specs:
--without-gui
Compiler throws multiple warnings; mostly
-Wdeprecated-copy
, but also one potentially dangerous-Wclass-memaccess
.The scary
memcpy()
problem:The ones causing most of the warnings and being repeated a lot during the compilation (in various references):
All the others referred either a bit less often, or occuring just once:
Those seem to be all I managed to catch and filter out/unify during the quickly scrolling compilation terminal. ;)
The text was updated successfully, but these errors were encountered: