Use != 0 instead of == 1 for check of masked value #380
Conversation
Recently [0] we added a check on a value masked against `SEQUENCE_LOCKTIME_DISABLE_FLAG`, this check uses `x == 1` but x can never be zero since `SEQUENCE_LOCKTIME_DISABLE_FLAG` is equal to `0x80000000`. Looking at the original commit it looks like we are trying to check for a value _higher_ than the flag value, so we can mask then use `x != 0` to see if the flag bit is enabled. Found by clippy: error: incompatible bit mask: `_ & 2147483648` can never be equal to `1` [0] commit: `commit a3dd9fe`
|
Oh, right 🤦. Good catch. ACK b747e51.
…-------- Original Message --------
On May 2, 2022, 5:38 AM, Tobin C. Harding wrote:
Recently [0] we added a check on a value masked against SEQUENCE_LOCKTIME_DISABLE_FLAG, this check uses x == 1 but x can never be zero since SEQUENCE_LOCKTIME_DISABLE_FLAG is equal to 0x80000000.
Looking at the original commit it looks like we are trying to check for a value higher than the flag value, so we can mask then use x != 0 to see if the flag bit is enabled.
Found by clippy:
error: incompatible bit mask: _ & 2147483648 can never be equal to 1
[0] commit: commit a3dd9fe
CC: ***@***.***(https://github.com/darosior), have I understood this correctly man?
---------------------------------------------------------------
You can view, comment on, or merge this pull request online at:
#380
Commit Summary
- [b747e51](b747e51) Use != 0 instead of == 1 for check of masked value
File Changes
([2 files](https://github.com/rust-bitcoin/rust-miniscript/pull/380/files))
- M [src/miniscript/types/extra_props.rs](https://github.com/rust-bitcoin/rust-miniscript/pull/380/files#diff-b3dad294504fb8705b45aeeeb9235b1bb6dd3aa52495684a936a59ee6cc5c876) (4)
- M [src/miniscript/types/mod.rs](https://github.com/rust-bitcoin/rust-miniscript/pull/380/files#diff-692f07910f91eef032ec530a82c31d6394e8346c0cf057c4ad8d7de3a2c89c24) (8)
Patch Links:
- https://github.com/rust-bitcoin/rust-miniscript/pull/380.patch
- https://github.com/rust-bitcoin/rust-miniscript/pull/380.diff
—
Reply to this email directly, [view it on GitHub](#380), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F24V6R67JIDI5XNGZ3VH5E37ANCNFSM5U23GXMA).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@apoelstra, does this make for trying out We should not enforce it on every CI run. But we can schedule CI runs every week Sunday night running the clippy CI on the latest tip? This way
We can debate the schedule period if there is interest. |
|
I am attempting to clear all the clippy warnings here and everywhere in our stack, I'm doing this in an attempt to get clippy running on CI for ever PR - I see no reason not to, is there some reason I'm missing? From my understanding the problems were all related to Rust 1.29, that's why I've waited until now to do this work. |
FTR, I am in favor of it. But several contributors don't always like the suggestions Clippy presents. It is yet another thing to check while submitting the pull request. And I am sure that we will end up with lots of cases where the last commit is just "Fix up Clippy lints". In addition to every commit compiling tests, contributors need to run Clippy on every commit which people can forget and would get annoyed whenever the CI yells at low priority things are false positives. Then we need to figure out how to disable some lints that are not annoying etc. As I said, I am in favor, I am just stating the reasons why people might oppose it. |
|
oh I forgot about how annoying it is going back over patch sets to fix clippy stuff. Thanks for reminding me, we definitely should find a solution that does not lead to that. |
|
Note one solution to this is to add a commit hook to git which fails to commit unless clippy passes. According to this SO you can commit a directory to rust-miniscript like: |
|
Sweeeet, thanks man. |
Recently [0] we added a check on a value masked against
SEQUENCE_LOCKTIME_DISABLE_FLAG, this check usesx == 1but x can never be zero sinceSEQUENCE_LOCKTIME_DISABLE_FLAGis equal to0x80000000.Looking at the original commit it looks like we are trying to check for a value higher than the flag value, so we can mask then use
x != 0to see if the flag bit is enabled.Found by clippy:
error: incompatible bit mask:
_ & 2147483648can never be equal to1CC: @darosior, have I understood this correctly man?
[0] commit:
commit a3dd9fe840c8cdae84e9d586cd9a9d6c195f0092