Closed Bug 1167025 Opened 4 years ago Closed 4 years ago

Assembler-x86-shared.h(2769) : warning C4805: '|' : unsafe mix of type 'bool' and type 'int' in operation


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox41 --- fixed


(Reporter: dholbert, Assigned: bbouvier)


(Blocks 1 open bug)



(1 file)

When building on Windows, we get multiple instances of this warning:
18:14:39     INFO -  Warning: C4805 in c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\js\src\jit\x86-shared\Assembler-x86-shared.h: '|' : unsafe mix of type 'bool' and type 'int' in operation
18:14:39     INFO -  c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\js\src\jit/x86-shared/Assembler-x86-shared.h(2769) : warning C4805: '|' : unsafe mix of type 'bool' and type 'int' in operation

(We get multiple instances because this is a .h file included in several places, presumably.)

Sample build log:

This warning is pointing to the following code:
> 2768     unsigned blendpsMask(bool x, bool y, bool z, bool w) {
> 2769         return x | (y << 1) | (z << 2) | (w << 3);
> 2770     }

And sure enough, "x" is a bool, and it's is getting bitwise-or'ed with a series of ints (bit-shifted bools) -- that's why MSVC is warning.

Dan, if you're sure this code is really doing what you want, could you perhaps do something to promote "x" to be an int (or unsigned) here, so that we don't get this build warning?

(strawman suggestion: "x << 0" might do the trick, and have some nice consistency with the other shifts on the same line)
Flags: needinfo?(sunfish)
It makes sense to keep booleans as part of this interface, as each value could correspond to "do we take the source value?" (true meaning yes, false meaning "no, take the destination value" -- not sure about source/destination order, but you get the idea :))

As a matter of fact, the only two possible values mean "yes" or "no", so boolean seems more adapted than unsigned, and having "x << 0" makes the most sense.
Flags: needinfo?(sunfish)
Attached patch p.patchSplinter Review
As explained in the previous comment.
Assignee: nobody → benj
Attachment #8608568 - Flags: review?(sunfish)
Comment on attachment 8608568 [details] [diff] [review]

Review of attachment 8608568 [details] [diff] [review]:

If that fixes the warning, it works for me.
Attachment #8608568 - Flags: review?(sunfish) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.