I think in coders/miff.c line 392, there is a cast missing.
It looks like quantum|=(*p++);
but should actually be quantum|=((unsigned int) *p++);
Did something other than code inspection cause you to notice this?
The type of p is 'unsigned char' and the type of 'quantum' is
'unsigned int'. I believe that the rules say that the value is
automatically cast to the type of the value it is assigned to. The
value is the least significant byte of a two-byte value. So
adding the cast should not change the result.
Some of the other casts were needed because the 8-bit value would be
lost entirely unless there is an upcast to provide a place for the
value when it is left/up shifted by 8.
However, I have no problem with adding additional casts if they
improve consistency and clarity. The code is not consistent since
there are other similar cases with the cast.
Thank you for clarification. I noticed it by skimming through the code and looking into the changeset 15991:bc99af93614d. Since it is the only occurrence that was not changed while the same case was changed on other lines, it looked to me it was accidentally forgotten rather than intentionally kept as is, although it has no impact.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thank you for clarification. I noticed it by skimming through the
code and looking into the changeset 15991:bc99af93614d. Since it is
the only occurrence that was not changed while the same case was
changed on other lines, it looked to me it was accidentally
forgotten rather than intentionally kept as is, although it has no
impact.
Source code inspections are always appreciated. Even better are
volunteers willing to help with development and testing.
On Wed, 27 Jul 2022, tiefensuche wrote:
Did something other than code inspection cause you to notice this?
The type of p is 'unsigned char' and the type of 'quantum' is
'unsigned int'. I believe that the rules say that the value is
automatically cast to the type of the value it is assigned to. The
value is the least significant byte of a two-byte value. So
adding the cast should not change the result.
Some of the other casts were needed because the 8-bit value would be
lost entirely unless there is an upcast to provide a place for the
value when it is left/up shifted by 8.
However, I have no problem with adding additional casts if they
improve consistency and clarity. The code is not consistent since
there are other similar cases with the cast.
Bob
Bob Friesenhahn
bfriesen@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer, http://www.GraphicsMagick.org/
Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt
Thank you for clarification. I noticed it by skimming through the code and looking into the changeset 15991:bc99af93614d. Since it is the only occurrence that was not changed while the same case was changed on other lines, it looked to me it was accidentally forgotten rather than intentionally kept as is, although it has no impact.
On Thu, 28 Jul 2022, tiefensuche wrote:
Source code inspections are always appreciated. Even better are
volunteers willing to help with development and testing.
Bob
Bob Friesenhahn
bfriesen@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer, http://www.GraphicsMagick.org/
Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt
I have implemented the suggested change as Mercurial changeset 16717:0a06f497d588. Thanks for bringing this issue to my attention.