Fix signed/unsigned comparison warnings in QuartzSupport.mm (in nsCARenderer::SetupRenderer & nsCARenderer::AttachIOSurface)

RESOLVED DUPLICATE of bug 924444

Status

()

Core
Graphics
RESOLVED DUPLICATE of bug 924444
5 years ago
4 years ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 years ago
gfx/2d is nearly warning-free -- my final try run before pushing bug 839269 hit some warnings in QuartzSupport.mm:
{
../../../../gfx/2d/QuartzSupport.mm:541:14: error: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
  if (aWidth == mUnsupportedWidth &&
      ~~~~~~ ^  ~~~~~~~~~~~~~~~~~
../../../../gfx/2d/QuartzSupport.mm:542:15: error: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
      aHeight == mUnsupportedHeight) {
      ~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~
../../../../gfx/2d/QuartzSupport.mm:807:32: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
    if (mIOSurface->GetWidth() != width || mIOSurface->GetHeight() != height) {
        ~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~
../../../../gfx/2d/QuartzSupport.mm:807:68: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
    if (mIOSurface->GetWidth() != width || mIOSurface->GetHeight() != height) {
                                           ~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~
../../../../gfx/2d/QuartzSupport.mm:943:20: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
  if (aX < 0 || aX >= ioWidth ||
                ~~ ^  ~~~~~~~
../../../../gfx/2d/QuartzSupport.mm:944:20: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
      aY < 0 || aY >= ioHeight) {
                ~~ ^  ~~~~~~~~
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=21444993&tree=Try
(Reporter)

Comment 1

5 years ago
Actually, I'm spinning the last two warnings off into their own bug, because they're clearly-innocuous and dead simple to fix w/ a static_cast.

The other ones are less clear, without knowing the details of this code -- they might be best-fixed with either some API changes and/or added range-checks.
(Reporter)

Comment 2

5 years ago
(spin-off bug for the last 2 warnings: bug 858304)
(Reporter)

Comment 3

5 years ago
Brief analysis, from skimming the code:

(In reply to Daniel Holbert [:dholbert] from comment #0)
> gfx/2d is nearly warning-free -- my final try run before pushing bug 839269
> hit some warnings in QuartzSupport.mm:
> {
> ../../../../gfx/2d/QuartzSupport.mm:541:14: error: comparison of integers of
> different signs: 'int' and 'uint32_t' (aka 'unsigned int')
> [-Werror,-Wsign-compare]
>   if (aWidth == mUnsupportedWidth &&
>       ~~~~~~ ^  ~~~~~~~~~~~~~~~~~
> ../../../../gfx/2d/QuartzSupport.mm:542:15: error: comparison of integers of
> different signs: 'int' and 'uint32_t' (aka 'unsigned int')
> [-Werror,-Wsign-compare]
>       aHeight == mUnsupportedHeight) {
>       ~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~
https://mxr.mozilla.org/mozilla-central/source/gfx/2d/QuartzSupport.mm?rev=9624c7fca53f&mark=541-542#533

For these two, we might want to make |aWidth| and |aHeight| unsigned values up-front. From a brief skim, it looks like some (maybe all?) of the callers will be passing in unsigned values anyway, which we're implicitly converting to signed, and then treating as unsigned.

> ../../../../gfx/2d/QuartzSupport.mm:807:32: error: comparison of integers of
> different signs: 'size_t' (aka 'unsigned long') and 'int'
> [-Werror,-Wsign-compare]
>     if (mIOSurface->GetWidth() != width || mIOSurface->GetHeight() !=
> height) {
>         ~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~
> ../../../../gfx/2d/QuartzSupport.mm:807:68: error: comparison of integers of
> different signs: 'size_t' (aka 'unsigned long') and 'int'
> [-Werror,-Wsign-compare]
>     if (mIOSurface->GetWidth() != width || mIOSurface->GetHeight() !=
> height) {
https://mxr.mozilla.org/mozilla-central/source/gfx/2d/QuartzSupport.mm?rev=9624c7fca53f&mark=807-807#790

For these two, we might want to just make |width| and |height| unsigned, too... they actually come from float values, and we don't currently do any sort of bounds-check (to see if they're nonnegative, or to see if they're in-bounds for an integer).  I suspect we might want to check (with an assertion, at least) that they're in the [0, UINT32_MAX) range, and then stick them in an unsigned variable...

I'm not immediately taking this bug; I'm sorta hoping that someone who knows this code & the relevant APIs (smichaud perhaps?) might be able to take a more educated crack at this.
OS: Linux → Mac OS X
Hardware: x86_64 → All
(Reporter)

Updated

5 years ago
Summary: Fix signed/unsigned comparison warnings in QuartzSupport.mm → Fix signed/unsigned comparison warnings in QuartzSupport.mm (in nsCARenderer::SetupRenderer & nsCARenderer::AttachIOSurface)
I'm starting a long weekend tomorrow.  So you might want to look for someone else.
(Reporter)

Comment 5

5 years ago
(no worries -- doesn't have to be fixed right away. If no one's gotten to it within a week or two, I might circle back; if you can get to it before then, I'd be much obliged, but no pressure. :))

Comment 6

4 years ago
Forward duping to Bug 924444 as it has a r+ patch attached.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 924444
(Reporter)

Comment 7

4 years ago
Thanks! Transferring dependencies to that bug.
No longer blocks: 187528, 839269
You need to log in before you can comment on or make changes to this bug.