refactor: clean up -Wlogical-op warning in wallet/scriptpubkeyman.cpp #19986
Conversation
|
Thanks @maskoficarus ! |
|
I'm not sure I get this. |
Good point. It shouldn't be needed. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits |
|
Thanks for squashing, however if we're going to merge this you'll need to use a commit message that is more useful/explanatory than the GitHub default. See CONTRIBUTING.md#committing-patches. |
This commit fixes #19912 by removing a check that always returned true. That check was causing a -Wlogical-op warning because it treated a constant int as though it were a boolean.
|
@achow101 As the author of 415afcc, would you mind reviewing this to make sure the the logic @maskoficarus suggests corresponds to what was originally intended? :) |
|
review ACK 95fedd3 Though, would be good to have instructions on how to reproduce the warning |
I was able to trigger this warning just by building with GCC 9.2.0 (on Gentoo Linux). |
|
@MarcoFalke gcc 9.3 which is the default compiler in Ubuntu 20.04 prints this diagnostic by default :) |
There was a problem hiding this comment.
ACK 95fedd3, tested on Linux Mint 20 (x86_64):
- master:
$ gcc --version
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ git rev-parse HEAD
b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05
$ make > /dev/null
wallet/scriptpubkeyman.cpp: In member function ‘virtual bool LegacyScriptPubKeyMan::Upgrade(int, bilingual_str&)’:
wallet/scriptpubkeyman.cpp:455:55: warning: logical ‘and’ applied to non-boolean constant [-Wlogical-op]
455 | if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) && CHDChain::VERSION_HD_CHAIN_SPLIT) {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- PR:
$ git rev-parse HEAD
95fedd33a23d6cb7542378afef229965f1ebfd68
$ make > /dev/null
# no output :)
…t/scriptpubkeyman.cp 95fedd3 refactor: Clean up -Wlogical-op warning (maskoficarus) Pull request description: This is a quick patch that fixes bitcoin#19912 . This change prevents a -Wlogical-op warning that occurs because we're treating a const int value as a boolean. There's no sense checking if a non-zero constant has a value, so I've removed the check. bitcoin#18836 also addresses the same warning, but has a larger scope and will require more review. This pull request will act as a patch to prevent this compile warning until 18836 is merged. ACKs for top commit: MarcoFalke: review ACK 95fedd3 hebasto: ACK 95fedd3, tested on Linux Mint 20 (x86_64): Tree-SHA512: 13a9d7f7cb472f4c22a01ca2f9771a75267ff769bdae9d0dc6b2c7f3b05369f6dfa859be2b172b39c15ede6c44cddf556380b3565e42850faa65ccd3fe6e175b
… exceptions 2f6fe4e ci: Build with --enable-werror by default, and document exceptions (Hennadii Stepanov) Pull request description: This PR prevents introducing of new compiler warnings in the master branch, e.g., #19986, #20162. ACKs for top commit: practicalswift: cr ACK 2f6fe4e: patch looks correct MarcoFalke: re-ACK 2f6fe4e 🏏 vasild: ACK 2f6fe4e Tree-SHA512: 23b5feb5bc472658c992d882ef61af23496f25adaa19f9c79bfaef5d2db273d44981aa93b1631a7d37cb58755283c1dacf3f2d68e501522d3fa8c965ab646d19
Summary: This commit fixes [[bitcoin/bitcoin#19912 | core#19912]] by removing a check that always returned true. That check was causing a -Wlogical-op warning because it treated a constant int as though it were a boolean. This is a backport of [[bitcoin/bitcoin#19986 | core#19986]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10538
This is a quick patch that fixes #19912 . This change prevents a -Wlogical-op warning that occurs because we're treating a const int value as a boolean. There's no sense checking if a non-zero constant has a value, so I've removed the check.
#18836 also addresses the same warning, but has a larger scope and will require more review. This pull request will act as a patch to prevent this compile warning until 18836 is merged.