Skip to content
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.

Sign up for GitHub

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

Jump to bottom

-Wlogical-op warning in wallet/scriptpubkeyman.cpp when building current master #19912

Closed
kristapsk opened this issue Sep 7, 2020 · 6 comments · Fixed by #19986
Closed

-Wlogical-op warning in wallet/scriptpubkeyman.cpp when building current master #19912

kristapsk opened this issue Sep 7, 2020 · 6 comments · Fixed by #19986
Labels
Bug

Comments

@kristapsk
Copy link
Contributor 

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) {
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  CXX      wallet/libbitcoin_wallet_a-wallet.o

$ gcc --version
gcc (Gentoo 9.2.0-r2 p3) 9.2.0

Related - #19015.
@kristapsk kristapsk added the Bug label Sep 7, 2020
@practicalswift practicalswift mentioned this issue Sep 8, 2020
Merged
@practicalswift
Copy link
Contributor

Thanks for reporting! Very nice to see the recently enabled -Wlogical-op pay off. Time for -Werror=logical-op?

@achow101
Copy link
Member

achow101 commented Sep 8, 2020

#18836 fixes this.

@practicalswift
Copy link
Contributor

@achow101 Thanks! I guess #18836 will need additional review time given its size/scope: would it make sense to patch this separately to get the fix in sooner?

@maskoficarus maskoficarus mentioned this issue Sep 20, 2020
Merged
@maskoficarus
Copy link

I've added a standalone fix in #19986 to get this resolved while #18836 gets sorted out.

@jonatack
Copy link
Contributor

Interesting. I compile with this flag and am not seeing this. Just reverified on both gcc and clang.

$ gcc --version
gcc (Debian 10.2.0-7) 10.2.0
$ clang --version
Debian clang version 12.0.0

@hebasto
Copy link
Member

hebasto commented Oct 18, 2020

gcc 10.2.1 (Fedora 32) -- no warnings
gcc 9.3.0 (Linux Mint 20 / Ubuntu Focal) -- a warning is emitted

hebasto pushed a commit to hebasto/bitcoin that referenced this issue Oct 18, 2020
@hebasto
refactor: Clean up -Wlogical-op warning
5de9689 
This commit fixes bitcoin#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.
fanquake added a commit that referenced this issue Oct 19, 2020
@fanquake
Merge #19986: refactor: clean up -Wlogical-op warning in wallet/scrip…
a1e0359 
…tpubkeyman.cp

95fedd3 refactor: Clean up -Wlogical-op warning (maskoficarus)

Pull request description:

  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.

ACKs for top commit:
  MarcoFalke:
    review ACK 95fedd3
  hebasto:
    ACK 95fedd3, tested on Linux Mint 20 (x86_64):

Tree-SHA512: 13a9d7f7cb472f4c22a01ca2f9771a75267ff769bdae9d0dc6b2c7f3b05369f6dfa859be2b172b39c15ede6c44cddf556380b3565e42850faa65ccd3fe6e175b
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Oct 19, 2020
@fanquake
Merge bitcoin#19986: refactor: clean up -Wlogical-op warning in walle…
cfc58de 
…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
@kristapsk kristapsk mentioned this issue Dec 18, 2020
Closed
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Nov 25, 2021
@PiRK
refactor: Clean up -Wlogical-op warning
a46f4c0 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: clean up -Wlogical-op warning in wallet/scriptpubkeyman.cpp
7 participants
@jonatack @achow101 @kristapsk @practicalswift @maskoficarus @hebasto and others