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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

(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.)

Comment 2

[Daniel Holbert [:dholbert]]

(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).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1: cast to unsigned, with assertion for good measure

Comment 4

[Daniel Holbert [:dholbert]]

Full Windows try run (to make sure we don't run afoul of the assertion anywhere):
 https://tbpl.mozilla.org/?tree=Try&rev=83969fa59c92

Comment 5

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28531e5859c9

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/28531e5859c9