Closed Bug 957904 Opened 6 years ago Closed 6 years ago

CompositorD3D11.cpp(431) : warning C4018: '<=' : signed/unsigned mismatch

Categories

(Core :: Graphics: Layers, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Build warnings:
{
c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/gfx/layers/d3d11/CompositorD3D11.cpp(431) : warning C4018: '<=' : signed/unsigned mismatch
c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/gfx/layers/d3d11/CompositorD3D11.cpp(432) : warning C4018: '<=' : signed/unsigned mismatch
}

Code sample:
> 422     D3D11_BOX srcBox;
[...]
> 430     const IntSize& srcSize = sourceD3D11->GetSize();
> 431     if (srcBox.right <= srcSize.width &&
> 432         srcBox.bottom <= srcSize.height) {
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#422

The member variables of D3D11_BOX are unsigned (type UINT), according to [1], whereas the member variables on IntSize are signed (int32_t), according to [2]. So we should really be casting one way or the other before comparing them.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ff476089%28v=vs.85%29.aspx
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Point.h#95
(Side note: the "[...]" from comment 0's code snippet was:

> 423     srcBox.left = aSourcePoint.x;
> 424     srcBox.top = aSourcePoint.y;
> 425     srcBox.front = 0;
> 426     srcBox.right = aSourcePoint.x + aRect.width;
> 427     srcBox.bottom = aSourcePoint.y + aRect.height;
> 428     srcBox.back = 0;
> 429 

Note that aSourcePoint is a gfx::IntPoint, and aRect is a gfx::IntRect.  If those happen to be negative, we'll run into trouble when sticking them in |srcBox|'s unsigned values...  We probably should have some assertions about those arguments being nonnegative, I'd imagine.)
(In reply to Daniel Holbert [:dholbert] from comment #0)
> So we should really be casting one way or the other before comparing them.

I'm going to assume we should cast to unsigned, because:
 * The signed values in question are from a rendering target's GetSize() method...
 * It seems reasonable to expect that rendering targets should have nonnegative sizes. (If we're rendering to a negatively-sized target, it sounds like something's gone very wrong.)

...and also, not a strong reason, but FWIW:
 * This is implicitly what's happening under the hood, already. (When an int and uint of the same size are compared, the int gets promoted to uint).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8357586 - Flags: review?(ncameron)
Attachment #8357586 - Flags: review?(ncameron) → review+
Full Windows try run (to make sure we don't run afoul of the assertion anywhere):
 https://tbpl.mozilla.org/?tree=Try&rev=83969fa59c92
https://hg.mozilla.org/mozilla-central/rev/28531e5859c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.