1226277 - "warning C4146: unary minus operator applied to unsigned type, result still unsigned" in AtomicOperations-x86-shared.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Looks like this code was added by bug 1142593.

js\src\jit/x86-shared/AtomicOperations-x86-shared.h(522) : warning C4146: unary minus operator applied to unsigned type, result still unsigned

The line in question:
http://hg.mozilla.org/mozilla-central/file/a523d4c7efe2/js/src/jit/x86-shared/AtomicOperations-x86-shared.h#l522

   520 MSC_FETCHADDOP(uint16_t, short, _InterlockedExchangeAdd16)
   521 MSC_FETCHADDOP(int32_t, long, _InterlockedExchangeAdd)
>> 522 MSC_FETCHADDOP(uint32_t, long, _InterlockedExchangeAdd)
   523 
   524 # undef MSC_FETCHADDOP

Comment 1

[Lars T Hansen [:lth]]

/me thinks that, perhaps, in the macro MSC_FETCHADDOP, it should be -(U)val and not (U)-val.

Comment 2

[Lars T Hansen [:lth]]

Attachment: fix MSVC warning with a cast

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8689910 [details] [diff] [review]
fix MSVC warning with a cast

Review of attachment 8689910 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I think we could use C++ coercions here and above.

::: js/src/jit/x86-shared/AtomicOperations-x86-shared.h
@@ +510,5 @@
>      }                                                                         \
>      template<> inline T                                                       \
>      js::jit::AtomicOperations::fetchSubSeqCst(T* addr, T val) {               \
>          static_assert(sizeof(T) <= 4, "not available for 8-byte values yet"); \
> +        return (T)xadd((U volatile*)addr, -(U)val);                           \

Any way you can use C++ coercions here for the coercion into T and the second U coercion?

return T(xadd((U volatile*) addr, -U(val))); \

Comment 4

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8689910 [details] [diff] [review]
> fix MSVC warning with a cast
> 
> Review of attachment 8689910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Any way you can use C++ coercions here for the coercion into T and the
> second U coercion?

It's a good point, but I will still land this as-is to minimize time spent on it today.

Comment 5

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7cc5ec688657e6d3711c1be1ca8d5db5119329
Bug 1226277 - fix MSVC warning with a cast.  r=bbouvier

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1e7cc5ec6886