Skip to content

Commit

Permalink
Fix MSVC unary minus on unsigned type warning
Browse files Browse the repository at this point in the history
Summary:
Negating an unsigned number is not mathematically meaningful, hence Visual Studio complains with this warning:
```
thrift\lib\cpp\util\cpp-util-headerswindows#header-mode-symlink-tree-only,headers\thrift\lib\cpp\util\varintutils-inl.h(203): warning C4146: unary minus operator applied to unsigned type, result still unsigned
thrift\lib\cpp\util\cpp-util-headerswindows#header-mode-symlink-tree-only,headers\thrift\lib\cpp\util\varintutils-inl.h(207): warning C4146: unary minus operator applied to unsigned type, result still unsigned
```
We can revise this code to clarify what the zig-zag operation is actually doing. Basically it decides whether to invert all bits depending on the value of `n & 1`

While less compact in C this does not change functionality nor performance on x86_64 or armv7, as confirmed by clang `-O3`, `-Os`, and `-Oz` generating identical instructions. Interestingly though, for aarch64 it generates sequences with one fewer instruction:
```
0000000000000000 <_Z11zigzagToI32j>:
   0:	12000008 	and	w8, w0, #0x1
   4:	4b0803e8 	neg	w8, w8
   8:	4a400500 	eor	w0, w8, w0, lsr #1
   c:	d65f03c0 	ret
-->
0000000000000000 <_Z11zigzagToI32j>:
   0:	13000008 	sbfx	w8, w0, #0, #1
   4:	4a400500 	eor	w0, w8, w0, lsr #1
   8:	d65f03c0 	ret
```

(Again with the identical optimization regardless of `-O3`, `-Os`, or `-Oz`.)

Reviewed By: yfeldblum

Differential Revision: D17789109

fbshipit-source-id: 1cf0977cd9369402051058c4fb0576505eb05970
  • Loading branch information
jdonald authored and facebook-github-bot committed Oct 8, 2019
1 parent c00ff8b commit e80ecd1
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
4 changes: 2 additions & 2 deletions thrift/lib/cpp/util/VarintUtils-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,11 @@ uint8_t writeVarint(Cursor& c, T value) {
}

inline int32_t zigzagToI32(uint32_t n) {
return (n >> 1) ^ -(n & 1);
return (n & 1) ? ~(n >> 1) : (n >> 1);
}

inline int64_t zigzagToI64(uint64_t n) {
return (n >> 1) ^ -(n & 1);
return (n & 1) ? ~(n >> 1) : (n >> 1);
}

constexpr inline uint32_t i32ToZigzag(const int32_t n) {
Expand Down
2 changes: 1 addition & 1 deletion thrift/lib/cpp2/protocol/nimble/ChunkRepr.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ inline std::uint32_t zigzagEncode(std::uint32_t n) {
}

inline std::uint32_t zigzagDecode(std::uint32_t n) {
return (n >> 1) ^ -(n & 1);
return (n & 1) ? ~(n >> 1) : (n >> 1);
}

template <ChunkRepr repr>
Expand Down

0 comments on commit e80ecd1

Please sign in to comment.