Re: [PATCH - for gui-staging] xpdf: revert uninitialized warning fix

[Michal H. <ms...@gm...>]

On Tue, Jan 31, 2012 at 10:18:52AM +0100, Jozef Misutka wrote:
> Dne 1/29/2012 10:12 PM, Michal Hocko napsal(a):
> >On Wed, Jan 25, 2012 at 06:28:17PM +0100, Michal Hocko wrote:
> >[...]
> >>diff --git a/src/xpdf/xpdf/Gfx.cc b/src/xpdf/xpdf/Gfx.cc
> >>index fe1c465..1a29de4 100644
> >>--- a/src/xpdf/xpdf/Gfx.cc
> >>+++ b/src/xpdf/xpdf/Gfx.cc
> >[...]
> >>@@ -2127,7 +2125,7 @@ void Gfx::doAxialShFill(GfxAxialShading *shading) {
> >>    double t0, t1, tt;
> >>    double ta[axialMaxSplits + 1];
> >>    int next[axialMaxSplits + 1];
> >>-  GfxColor color0, color1;
> >>+  GfxColor color0 = {}, color1 = {};
> >>    int nComps;
> >>    int i, j, k, kk;
> >>
> >>What happens if we really do not initialize them? Isn't this just
> >>papering over a real problem?
> >OK, I have double checked the change and I think we can never end up
> >using those two uninitialized. Unless I missed something the following
> >patch should be applied to gui-staging.
> >---
> >> From 22a549d4db5e1ed6b51db1d985e46f60e30c8b73 Mon Sep 17 00:00:00 2001
> >From: Michal Hocko<ms...@gm...>
> >Date: Sun, 29 Jan 2012 22:04:54 +0100
> >Subject: [PATCH - for gui-staging] xpdf: revert uninitialized warning fix
> >
> >It has been observed that some compilers warn about Gfx::doAxialShFill
> >not initializing color0 and color1. After a code inspection it turned
> >out that they will never be used uninitialized because
> >GfxShading::getColor always set the given color parameter.
> >color0 is initialized unconditionally and color1 might end up
> >uninitialized only if we skipped while (j>  i + 1) loop which is not
> >possible because j = next[0] = axialMaxSplits / 2 (which is 128).
> >
> >Let's revert the fix to be closer to the master branch.
> 
> I totally disagree - maybe you've inspected the code manually and
> found it to be correct (maybe you've just made a mistake); 

and maybe my compiler did the same mistake (no warning with -Wall or
with explicit -Wuninitialized). Hey common. Either prove it wrong or
stop this kind "let's shut up the compiler and paper over real bugs".

> however,
> - there are people out there who compile pdfedit and those warnings
> can cost them their time,
> - it is unproffesional,

Papering over real bugs by blind initialization is unproffesional. Have
you checked what would happen if you just intialize those values? I
have! And you would just get random values same as with unitialized ones
(the only difference is that if you initialize you will get more
constant crap.)

> - we are lost in warnings so we should minimise them to catch only
> the important ones,

We have thousands of warnings that come from char * -> const char *
conversion. No change in that...

> - somebody will change a function later and will rely on the fact
> that it is initialized and we are in big trouble,

It would have to change the initialization as well, don't you think?
Otherwise it would be consistently buggy.

> 
> (btw: this is exactly why gui-jm was created)

Sorry but this is just bull crap. I haven't pushed anything to
pdfedit-staging yet. This has been posted for review and you haven't
pointed _any_ single technical argument. Please go and look into the
code. I am not saying I didn't make a mistake and I am open to push the
initialization fix if you back it by a technical argument.

> jm
> 
> >---
> >  src/xpdf/xpdf/Gfx.cc |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> >diff --git a/src/xpdf/xpdf/Gfx.cc b/src/xpdf/xpdf/Gfx.cc
> >index a64454b..e1b073d 100644
> >--- a/src/xpdf/xpdf/Gfx.cc
> >+++ b/src/xpdf/xpdf/Gfx.cc
> >@@ -2127,7 +2127,7 @@ void Gfx::doAxialShFill(GfxAxialShading *shading) {
> >    double t0, t1, tt;
> >    double ta[axialMaxSplits + 1];
> >    int next[axialMaxSplits + 1];
> >-  GfxColor color0 = {}, color1 = {};
> >+  GfxColor color0, color1;
> >    int nComps;
> >    int i, j, k, kk;
> >

-- 
Michal Hocko