refactor: Avoid unary minus on unsigned integers #1293

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:230430-minus changing 9 files +17 −18

hebasto commented at 4:59 PM on April 30, 2023: member

The assembly code remains unchanged.

In addition, this PR enables MSVC warning C4146 for the entire codebase.
ci: Treat all compiler warnings as errors in "Windows (VS 2022)" task b2e29e43d0
hebasto renamed this:
~~ci: Treat all compiler warnings as errors in "Windows (VS 2022)" task~~
refactor: Avoid unary minus on unsigned integers
on Apr 30, 2023
hebasto marked this as a draft on Apr 30, 2023
hebasto force-pushed on Apr 30, 2023
hebasto marked this as ready for review on Apr 30, 2023
real-or-random commented at 8:45 AM on May 2, 2023: contributor

I think rewriting -x to (0 - x) is a bit nicer than `(~x + 1).

With that change applied, I think I'm literally ~0 on that PR: On the one hand, changing the code to make the compiler happy is not great... But perhaps having a clean build with just /W2 is good.

refactor: Avoid unary minus on unsigned integers

The assembly code remains unchanged.

In addition, this change enables MSVC warning C4146 for the entire
codebase.
See: https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4146

e0e3874c8d

hebasto force-pushed on May 2, 2023
hebasto commented at 12:30 PM on May 2, 2023: member

I think rewriting -x to (0 - x) is a bit nicer than `(~x + 1).

Done.

On the one hand, changing the code to make the compiler happy is not great...

I agree with you.

But perhaps having a clean build with just /W2 is good.

That is my point.
jonasnick commented at 3:14 PM on May 11, 2023: contributor

Having to change perfectly fine -x to 0-x seems a bit annoying. I can imagine that this will confuse potential future reviewers. I don't think that's worth it.
sipa commented at 3:20 PM on May 11, 2023: contributor

Concept NACK, I think the resulting code is harder to read.

The warning in my opinion is silly (at least for our codebase), so the proper action is the disable the warning, not work around it.
hebasto closed this on May 11, 2023
hebasto deleted the branch on Jun 6, 2023

Contributors