Remove dead alpha_bits in nsPNGDecoder.cpp

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: njn, Assigned: glennrp+bmo)

Tracking

Trunk
mozilla19
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
GCC tells me this:

nsPNGDecoder.cpp: In static member function ‘static void mozilla::image::nsPNGDecoder::info_callback(png_structp, png_infop)’:
/home/njn/moz/mi3/image/decoders/nsPNGDecoder.cpp:612:11: warning: variable ‘alpha_bits’ set but not used [-Wunused-but-set-variable]

There's a whole complex series of nested statements (including a loop!) to compute alpha_bits.  Quite odd, but definitely dead.  (It's also conceivable that alpha_bits should be used subsequently, but I'm not qualified to determine that.)

I haven't run this past the try server yet, if I get r+ I'll do that before landing.
Attachment #614260 - Flags: review?(jmuizelaar)
This is related to Alfred's patch to bug #517713 (which keeps going dormant for some reason).
I agree that the code can be removed.  Once upon a time we made use of the alpha_bits variable but not any more.
Assignee: nobody → glennrp+bmo
The code cannot be removed if my patch is to be applied. In that case, the alphaBit/Byte check is required.

The patch in bug 517713 is dormant, because it requires superreview...
Who wants to take that on?
(In reply to Alfred Kayser from comment #3)
> The code cannot be removed if my patch is to be applied. In that case, the
> alphaBit/Byte check is required.
>

That's correct.  If/when Alfred's patch is landed after this one, it'll have to restore the
alpha_bits code.  The reason for storing alpha_bits is that premultiplying can be skipped later on, and finding alpha_bits is pretty quick compared to examining the entire alpha channel.  If Alfred's patch is landed first, then this one becomes obsolete.
Comment on attachment 614260 [details] [diff] [review]
patch

Sure. We can add something like this back if needed.
Attachment #614260 - Flags: review?(jmuizelaar) → review+
Duplicate of this bug: 802484
Looks like the patch in bug 517713 has some review feedback waiting to be addressed before it can land.  When another patch is ready in that bug, it can easily restore the otherwise-dead code here. 

So -- it looks like the patch here is reviewed and needs-landing.  Can we check this in?
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
> So -- it looks like the patch here is reviewed and needs-landing.  Can we
> check this in?

Is there much point removing code that a subsequent patch will just restore?  I suppose it might make us faster in the interim, and will avoid the warning -- dholbert, is that your desire?
Depends on how soon the subsequent patch will be landing. :)

In the meantime, I believe this is the only warning keeping this directory from being flagged as FAIL_ON_WARNINGS (as noted in bug 802490 comment 0), so it's holding us back from automatically preventing build warnings elsewhere in this file & directory.
Ok!  I'll land a patch that comments out the code in question and leaves a pointer to bug 517713.
...though I suppose we could also add
  //  This code is currently unused, but it's needed for bug 517713, when it's ready to land:
  #ifdef 0
   [... DEAD CODE ...]
  #endif 
to preserve hg blame etc. while still disabling the code for now.  Maybe that'd be better?
er #if 0 (not #ifdef)
Good idea.
https://hg.mozilla.org/mozilla-central/rev/357a60386b52
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.