Skip to content

Fix msvc compiler error C4146 (minus operator applied to unsigned type) #9916

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
Merged
merged 2 commits into from
Mar 9, 2017
Merged

Fix msvc compiler error C4146 (minus operator applied to unsigned type) #9916

merged 2 commits into from
Mar 9, 2017

Conversation

kobake
Copy link
Contributor

@kobake kobake commented Mar 4, 2017

On msvc14, the error C4146 (unary minus operator applied to unsigned type, result still unsigned) had been occured.
Minus operator should be applied to signed type.

laanwj
laanwj reviewed Mar 4, 2017
View reviewed changes
src/bloom.cpp Outdated
uint64_t nGenerationMask1 = -(uint64_t)(nGeneration & 1);
uint64_t nGenerationMask2 = -(uint64_t)(nGeneration >> 1);
uint64_t nGenerationMask1 = (uint64_t)(-(int64_t)(nGeneration & 1));
uint64_t nGenerationMask2 = (uint64_t)(-(int64_t)(nGeneration >> 1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works around the error, but I don't think it's very elegant. What about just formulating it explicitly:

        uint64_t nGenerationMask1 = ~(uint64_t)(nGeneration & 1) + 1;
        uint64_t nGenerationMask2 = ~(uint64_t)(nGeneration >> 1) + 1;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even just:

        uint64_t nGenerationMask1 = 0 - (uint64_t)(nGeneration & 1);
        uint64_t nGenerationMask2 = 0 - (uint64_t)(nGeneration >> 1);

Not sure this is any more readable than the above though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I confirmed three ways work well as same in my local msvc14.
The latter you show is simple but not feel as explicit bit calculation.
I like the former that is readable clearly as two's complement formula, then I'll adopt it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied. (commit --amend && push -f)

@kobake kobake force-pushed the fix-minus-operator-target-for-msvc-c4146 branch from 170887a to 3f0a0cf Compare March 4, 2017 10:06
@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 4, 2017 •
edited
Loading

ugh. MSVC produces the most absurd errors for perfectly valid, strictly conforming code. I don't have a problem with silencing it in this way that this PR currently does it (though IMO it makes it less clear).

The prior fix of casting to a signed type can actually result in undefined behavior (at least generally if not actually in our case due to the sizes we use). a=INT_MIN; a = -a; is undefined. (highlighting the foolishness of MSFT's war on bit manipulation. :P -- as someone trying to address this warning in an 'obvious way' would potentially introduce undefined behavior as a result!)

@laanwj
Copy link
Member

laanwj commented Mar 4, 2017

Agreed, that this is necessary at all is silly. There's no ambiguity here. Unary minus -x equivalent to 0-x which is well-defined also for unsigned values which are defined to wrap around.

@kobake
Copy link
Contributor Author

kobake commented Mar 5, 2017 •
edited
Loading

Yes, we should take care not to meet undefined behavior.
Now in this case, even if nGeneration has INT_MIN value, it's no problem because the target values to be applied two's complement formula are "(nGeneration & 1)" and "(nGeneration >> 1)"; they are 32bit values and their most significant bit is always zero, of course it's true even after the cast.

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 5, 2017

https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4146

Seems to indicate that 0-(uint64_t)(nGeneration & 1); will silence it-- if so I think that would be preferable.

@kobake
Copy link
Contributor Author

kobake commented Mar 6, 2017

OK, I just understood. I'll adopt 0 - x style.
FYI, msvc14 (vs2015) treats C4146 not as warning but as error by default because of /sdl option.

@kobake kobake force-pushed the fix-minus-operator-target-for-msvc-c4146 branch from 3f0a0cf to 40aca33 Compare March 6, 2017 03:17
@laanwj
Copy link
Member

laanwj commented Mar 6, 2017

utACK 40aca33

@laanwj laanwj force-pushed the fix-minus-operator-target-for-msvc-c4146 branch from 40aca33 to ac3cb48 Compare March 6, 2017 17:37
@laanwj
Copy link
Member

laanwj commented Mar 6, 2017

Amended the commit message to put a newline between the title and body, otherwise git log in short formats doesn't work as expected.

@kobake
Fix msvc compiler error C4146 (minus operator applied to unsigned type)
292112f 
On msvc14, the compiler error C4146 (unary minus operator applied to unsigned type, result still unsigned) had been occured.
Use '0 - x' styled formula instead of '-x' so as to fix the error.
@kobake kobake force-pushed the fix-minus-operator-target-for-msvc-c4146 branch from ac3cb48 to 292112f Compare March 6, 2017 17:43
@kobake
Copy link
Contributor Author

kobake commented Mar 6, 2017 •
edited
Loading

Oh, I see. I've inserted a new line as you say. Thanks for helpful tips.

@kobake
Copy link
Contributor Author

kobake commented Mar 7, 2017

I also fixed another C4146 error in the commit below that is about unit test.
kobake@3c73d82

Should I contain the commit in this PR #9916, or create an another PR for the commit?

@laanwj
Copy link
Member

laanwj commented Mar 8, 2017

It's another minor thing, I'd say add it here.

@kobake
Fix msvc compiler error C4146 (unary minus operator applied to unsign…
8e0720b 
…ed type)

On msvc14, int literal '-2147483648' is invalid, because '2147483648' is unsigned type and cant't apply minus operator to unsigned type.
To define the int literal correctly, use '-2147483647 - 1' formula that is also used to define INT_MIN in limits.h.
@kobake
Copy link
Contributor Author

kobake commented Mar 8, 2017

Thanks, I added.

@laanwj
Copy link
Member

laanwj commented Mar 8, 2017

re-UTack 8e0720b

@paveljanik
Copy link
Contributor

utACK 8e0720b

@laanwj laanwj merged commit 8e0720b into bitcoin:master Mar 9, 2017
laanwj added a commit that referenced this pull request Mar 9, 2017
@laanwj
Merge #9916: Fix msvc compiler error C4146 (minus operator applied to…
b403ec5 
… unsigned type)

8e0720b Fix msvc compiler error C4146 (unary minus operator applied to unsigned type) (kobake)
292112f Fix msvc compiler error C4146 (minus operator applied to unsigned type) (kobake)

Tree-SHA512: 25f408daf7bf9ffe4b8b4bd62f6f6d326219189a9faf8f8c0a135c5a0cb0511af765aa2b6087a091c8863c701289bda49a2379b00cd9b10854d316a5c3fc3f8e
@dagurval dagurval mentioned this pull request Jan 15, 2018
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 21, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9916: Fix msvc compiler error C4146 (minus operator app…
0d5966b 
…lied to unsigned type)

8e0720b Fix msvc compiler error C4146 (unary minus operator applied to unsigned type) (kobake)
292112f Fix msvc compiler error C4146 (minus operator applied to unsigned type) (kobake)

Tree-SHA512: 25f408daf7bf9ffe4b8b4bd62f6f6d326219189a9faf8f8c0a135c5a0cb0511af765aa2b6087a091c8863c701289bda49a2379b00cd9b10854d316a5c3fc3f8e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 27, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9916: Fix msvc compiler error C4146 (minus operator app…
8c7a2c6 
…lied to unsigned type)

8e0720b Fix msvc compiler error C4146 (unary minus operator applied to unsigned type) (kobake)
292112f Fix msvc compiler error C4146 (minus operator applied to unsigned type) (kobake)

Tree-SHA512: 25f408daf7bf9ffe4b8b4bd62f6f6d326219189a9faf8f8c0a135c5a0cb0511af765aa2b6087a091c8863c701289bda49a2379b00cd9b10854d316a5c3fc3f8e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9916: Fix msvc compiler error C4146 (minus operator app…
4ff6560 
…lied to unsigned type)

8e0720b Fix msvc compiler error C4146 (unary minus operator applied to unsigned type) (kobake)
292112f Fix msvc compiler error C4146 (minus operator applied to unsigned type) (kobake)

Tree-SHA512: 25f408daf7bf9ffe4b8b4bd62f6f6d326219189a9faf8f8c0a135c5a0cb0511af765aa2b6087a091c8863c701289bda49a2379b00cd9b10854d316a5c3fc3f8e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 26, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9916: Fix msvc compiler error C4146 (minus operator app…
e5c4a67 
…lied to unsigned type)

8e0720b Fix msvc compiler error C4146 (unary minus operator applied to unsigned type) (kobake)
292112f Fix msvc compiler error C4146 (minus operator applied to unsigned type) (kobake)

Tree-SHA512: 25f408daf7bf9ffe4b8b4bd62f6f6d326219189a9faf8f8c0a135c5a0cb0511af765aa2b6087a091c8863c701289bda49a2379b00cd9b10854d316a5c3fc3f8e
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Mar 9, 2019
@laanwj @UdjinM6
Merge bitcoin#9916: Fix msvc compiler error C4146 (minus operator app…
89f283f 
…lied to unsigned type)

8e0720b Fix msvc compiler error C4146 (unary minus operator applied to unsigned type) (kobake)
292112f Fix msvc compiler error C4146 (minus operator applied to unsigned type) (kobake)

Tree-SHA512: 25f408daf7bf9ffe4b8b4bd62f6f6d326219189a9faf8f8c0a135c5a0cb0511af765aa2b6087a091c8863c701289bda49a2379b00cd9b10854d316a5c3fc3f8e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 10, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9916: Fix msvc compiler error C4146 (minus operator app…
88db8d6 
…lied to unsigned type)

8e0720b Fix msvc compiler error C4146 (unary minus operator applied to unsigned type) (kobake)
292112f Fix msvc compiler error C4146 (minus operator applied to unsigned type) (kobake)

Tree-SHA512: 25f408daf7bf9ffe4b8b4bd62f6f6d326219189a9faf8f8c0a135c5a0cb0511af765aa2b6087a091c8863c701289bda49a2379b00cd9b10854d316a5c3fc3f8e
@kobake kobake deleted the fix-minus-operator-target-for-msvc-c4146 branch September 28, 2020 09:54
@str4d str4d mentioned this pull request Mar 5, 2021
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Mar 5, 2021
@zkbot
Auto merge of #5026 - str4d:bloom-filter-backports, r=<try>
be45961 
Backport bloom filter improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7113
- bitcoin/bitcoin#7818
  - Only the second commit (to resolve conflicts).
- bitcoin/bitcoin#7934
- bitcoin/bitcoin#8655
  - Partial backport to help resolve conflicts.
- bitcoin/bitcoin#9060
- bitcoin/bitcoin#9223
- bitcoin/bitcoin#9644
  - Partial backport to help resolve conflicts.
- bitcoin/bitcoin#9916
- bitcoin/bitcoin#9750
- bitcoin/bitcoin#13176
- bitcoin/bitcoin#13948
- bitcoin/bitcoin#16073
- bitcoin/bitcoin#18670
- bitcoin/bitcoin#18806
  - Reveals upstream's covert fix for CVE-2013-5700.
- bitcoin/bitcoin#19968
zkbot added a commit to zcash/zcash that referenced this pull request Apr 15, 2021
@zkbot
Auto merge of #5026 - str4d:bloom-filter-backports, r=str4d
78de0cd 
Backport bloom filter improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7113
- bitcoin/bitcoin#7818
  - Only the second commit (to resolve conflicts).
- bitcoin/bitcoin#7934
- bitcoin/bitcoin#8655
  - Partial backport to help resolve conflicts.
- bitcoin/bitcoin#9060
- bitcoin/bitcoin#9223
- bitcoin/bitcoin#9644
  - Partial backport to help resolve conflicts.
- bitcoin/bitcoin#9916
- bitcoin/bitcoin#9750
- bitcoin/bitcoin#13176
- bitcoin/bitcoin#13948
- bitcoin/bitcoin#16073
- bitcoin/bitcoin#18670
- bitcoin/bitcoin#18806
  - Reveals upstream's covert fix for CVE-2013-5700.
- bitcoin/bitcoin#19968
@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.
Reviewers

@laanwj laanwj laanwj left review comments

Assignees
No one assigned
Labels
Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kobake @gmaxwell @laanwj @paveljanik @fanquake