(In reply to comment #0)

> I do not have the knowledge to decide if this code is really correct
> but on a first glance the many "*pcmyk++" therein look somehow strange
> at least to me.
> 
> Therefore - to be on the safe side - I would like if you could have
> a look and verify if it is really correct.

Yes its correct.

This means to make the compiler happy I can change it to:
------------------------------------------------------------------------
                cmy = ( ((255 - *pcmyk) * lum_red_weight) +
                        ((255 - *pcmyk) * lum_green_weight) +
                        ((255 - *pcmyk) * lum_blue_weight) +
                        (lum_all_weights / 2))
                        / lum_all_weights;
                pcmyk++;
                pcmyk++;
                pcmyk++;
------------------------------------------------------------------------

Or is actually this meant:
------------------------------------------------------------------------
                cmy = ((255 - *pcmyk++) * lum_red_weight);
                cmy += ((255 - *pcmyk++) * lum_green_weight);
                cmy += ((255 - *pcmyk++) * lum_blue_weight);
                cmy += (lum_all_weights / 2);
                cmy /= lum_all_weights;
------------------------------------------------------------------------

Ken is wrong - the code is definitely wrong.

(In reply to comment #2)
> This means to make the compiler happy I can change it to:
> ------------------------------------------------------------------------
>                 cmy = ( ((255 - *pcmyk) * lum_red_weight) +
>                         ((255 - *pcmyk) * lum_green_weight) +
>                         ((255 - *pcmyk) * lum_blue_weight) +
>                         (lum_all_weights / 2))
>                         / lum_all_weights;
>                 pcmyk++;
>                 pcmyk++;
>                 pcmyk++;
> ------------------------------------------------------------------------

Your code would be even worse. A correct version would be:

>                 cmy  = ((255 - *pcmyk++) * lum_red_weight);
>                 cmy += ((255 - *pcmyk++) * lum_green_weight);
>                 cmy += ((255 - *pcmyk++) * lum_blue_weight);
>                 cmy  = (cmy + (lum_all_weights / 2)) / lum_all_weights;

(subject to cmy being the right type).

I will get this fixed.

Fixed in:

commit 621b010d1a6e12f7754508fee8a860345b4f84df
Author: Robin Watts <robin.watts@artifex.com>
Date:   Thu Aug 29 16:47:29 2013 +0100

    Bug 694546: Fix undefined behaviour in gdevpbm.

    Thanks to jsmeix for reporting this.

If you know of any other such warnings, please let us know! Thanks.

I addition to our autometed checks in our openSUSE build system
I searched for "undefined" in all our build log files for all
our various build targets (openSUSE_Factory openSUSE_12.3 openSUSE_12.2
openSUSE_12.1 SLE_11_SP3 SLE_11_SP2 SLE_11_SP1 SLE_11 where each
is built both for 32 bit i586 and 64 bit x86_64 architecture.
In our various build targets we use several GCC versions:
gcc-4.8 (openSUSE_Factory)
gcc-4.7 (openSUSE_12.3 openSUSE_12.2)
gcc-4.6 (openSUSE_12.1)
gcc-4.3 (SLE_11_SP3 SLE_11_SP2 SLE_11_SP1 SLE_11)

I found nothing.

This means with your commit 621b010d1a6e12f7754508fee8a860345b4f84df
our GCC versions do no longer report something as undefined.


Robin Watts,

I have a question regarding my comment#2
and your commit 621b010d1a6e12f7754508fee8a860345b4f84df
versus your comment#3:

Your commit 621b010d1a6e12f7754508fee8a860345b4f84df
is what I suggested as "is actually this meant" in comment#2
but in your comment#3 your "correct version" is different
any there you wrote "Your code would be even worse" and
"subject to cmy being the right type".

Now I am puzzled.

I am not an expert for those subtle details in C
but I think that my "make the compiler happy" code
is not worse but explicit code for what actually happens.
As far as I understand it "cmy = ... *pcmyk++ ... ;" results
that pcmyk gets incremented at the next sequence point and
as far as I see the next sequence point here is ';'.

Furthermore I think that my "is actually this meant" code
(and your commit 621b010d1a6e12f7754508fee8a860345b4f84df)
and your "correct version" result the same value in cmy.

I would appreciate a short note whether I am right or wrong.
Many thanks in advance!

(In reply to comment #5)
> This means with your commit 621b010d1a6e12f7754508fee8a860345b4f84df
> our GCC versions do no longer report something as undefined.

Thanks. I checked the warnings here from our build systems, and that was the only example I could find flagged up too.

> I am not an expert for those subtle details in C
> but I think that my "make the compiler happy" code
> is not worse but explicit code for what actually happens.

OK, so the original code was:

  cmy = ( ((255 - *pcmyk++) * lum_red_weight) +
          ((255 - *pcmyk++) * lum_green_weight) +
          ((255 - *pcmyk++) * lum_blue_weight) +
          (lum_all_weights / 2))
        / lum_all_weights;

Clearly the author expected the code to be evaluated from left to right, where each *pcmyk++ would be executed before the next. This would result in the R value being read, then the G value, then the B value, and the pointer being left pointing to the next B value. So the author hoped that the code would actually do:

  int R = *pcmyk++;
  int G = *pcmyk++;
  int B = *pcmyk++;
  cmy = ( ((255 - R) * lum_red_weight) +
          ((255 - G) * lum_green_weight) +
          ((255 - B) * lum_blue_weight) +
          (lum_all_weights / 2))
        / lum_all_weights;

You suggested that we could use:

                cmy = ( ((255 - *pcmyk) * lum_red_weight) +
                        ((255 - *pcmyk) * lum_green_weight) +
                        ((255 - *pcmyk) * lum_blue_weight) +
                        (lum_all_weights / 2))
                        / lum_all_weights;
                pcmyk++;
                pcmyk++;
                pcmyk++;

which would actually have been the same as doing:

  int R = *pcmyk;
  cmy = ( ((255 - R) * lum_red_weight) +
          ((255 - R) * lum_green_weight) +
          ((255 - R) * lum_blue_weight) +
          (lum_all_weights / 2))
        / lum_all_weights;
  pcmyk += 3;

Clearly that's not what the original author intended as the G and B values are completely ignored.

> I would appreciate a short note whether I am right or wrong.
> Many thanks in advance!

Hopefully this makes it clear. Thanks again for bringing this to our attention.

We (i.e. Robin Watts and I) have the same understanding of the code.

The only thing that is perhaps not yet fully clear is
how my comment#2 was meant.

My "make the compiler happy" code was in response to
Ken Sharp's statement that the original code is correct.

If the original code was correct,
then I could change it to my "make the compiler happy" code.

I wrote the "make the compiler happy" code to make it
explicitly clear what the original code actually does.

Because I assumed that the original code was not correct
I added the "is actually this meant" code in comment#2

My "is actually this meant" code was meant as suggestion
what you may use (if the original code is not correct) and
in your commit 621b010d1a6e12f7754508fee8a860345b4f84df
you use it so finally everything is now clear and resolved.

Many thanks for your time to explain it to me.