If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 45

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: lth)

Tracking

Trunk
mozilla45
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Assignee: nobody → lhansen
(Assignee)

Comment 1

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

Comment 2

2 years ago
Created attachment 8689910 [details] [diff] [review]
fix MSVC warning with a cast
Attachment #8689910 - Flags: review?(benj)
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))); \
Attachment #8689910 - Flags: review?(benj) → review+
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

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

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e7cc5ec6886
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.