Lenard,
Thanks a lot! :) I incorporated your fix into the baseline code, and there
just needed to be some slight modifications for the other conversion
functions, long, int, float, in addition to float and oct. I had initially
considered LONG_MAX, but I wasn't sure what it would do, and didn't know if
it would break 32 bit code. Now I know. Thanks. :) Here's an updated patch
that covers all the conversion functions.
-Tyler
On Fri, May 1, 2009 at 9:34 PM, Lenard Lindstrom <[email protected]> wrote:
> Hi,
>
> Yes, I was using the second patch which only modifies color.c. However the
> 'L's are necessary on 32 bit machines since 0xCC00CC00 can only be
> represented as a Python long. So try this patch. It keeps the "L"s but
> replaces INT_MAX with LONG_MAX, which is the proper test for a Python int
> (*).
>
> Lenard
>
> (*) ints are longs and longs are, well, something else.
>
> Tyler Laing wrote:
>
>> Are you using the second patch I provided? I had to make some
>> modifications initially to the color_test.py, which I had a feeling would
>> break on 32 bit machines. The second patch avoids modifying color_test.py,
>> and instead does not cast the results from hex and oct as longs, which was
>> adding a 'L' to the end of the result, but rather as ints every time.
>>
>> -Tyler
>>
>> On Fri, May 1, 2009 at 8:41 PM, Lenard Lindstrom <[email protected]<mailto:
>> [email protected]>> wrote:
>>
>> Hi Tyler,
>>
>> This patch breaks things for 32bit machines. Give me a few minutes
>> and I will see if I can cook something up.
>>
>> Lenard
>>
>>
>> Tyler Laing wrote:
>>
>> Rene,
>>
>> Oops, about the windows line ends. I'll make sure that doesn't
>> happen again. Sorry about that. I do have SVN write access
>> now, I just wanted to start with the patch first.
>>
>> I honestly don't know much about pygame.color, so I can't help
>> there.
>>
>> However, if we never want to return a long, then, we can change:
>>
>> /**
>> * oct(color)
>> */
>> static PyObject*
>> _color_oct (PyColor *color)
>> {
>> char buf[100];
>> unsigned long tmp = ((long)color->r << 24) +
>> ((long)color->g << 16) + ((long)color->b << 8) +
>> color->a;
>> if (tmp < INT_MAX)
>> PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
>> else
>> PyOS_snprintf (buf, sizeof (buf), "0%loL", tmp);
>> return PyString_FromString (buf);
>> }
>>
>> to
>> static PyObject*
>> _color_oct (PyColor *color)
>> {
>> char buf[100];
>> unsigned long tmp = ((long)color->r << 24) +
>> ((long)color->g << 16) + ((long)color->b << 8) +
>> color->a;
>> if (tmp < INT_MAX)
>> PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
>> else
>> PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
>> return PyString_FromString (buf);
>> }
>>
>> as an example, where we remove the 'L' from the else branch.
>>
>> I've got a fixed patch.diff now, where we don't return longs,
>> and its a lot smaller, because color_test.py does not need the
>> fixes now.
>>
>> I only have one error in the tests, and that results from not
>> having the right permissions, which is solved by a simple sudo.
>>
>> -Tyler
>>
>> On Fri, May 1, 2009 at 6:59 PM, René Dudfield
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>> wrote:
>>
>> Cool, thanks.
>>
>> I think we can look applying this patch after Lenard has
>> merged his
>> py3k stuff back in... so we don't get any many conflicts. As I
>> noticed he's already touched the color.c.
>>
>> We shouldn't ever be returning a long I don't think. So
>> perhaps we
>> can do a conversion from python long to python int at the
>> end of the
>> functions.
>>
>> For endian checking, you can use this define...
>>
>> #if SDL_BYTEORDER == SDL_LIL_ENDIAN
>> // lil endian code here...
>> #else
>> // big endian code here...
>> #endif
>>
>> However, I'm not positive the color code *should* be endian
>> safe...
>> without using the map/unmap methods on Surface for example.
>> However I
>> think maybe it should be. We will have to study how it is
>> being used
>> at the moment... and how the old behaviour worked(as some
>> code might
>> rely on it being endian unsafe).
>>
>>
>> ps. with patches it's good to check them to make sure
>> they're only
>> for the change you made... not for other issues at the same
>> time. eg,
>> there were lots of windows end of line characters in the
>> color test
>> before, and your patch removes them.
>>
>> pps. just a note, that a request has been put in to the
>> lovely server
>> administrators at seul.org <http://seul.org>
>> <http://seul.org> for adding svn
>>
>> accounts for the all the
>> GSOC peoples. So you should get your svn account within a
>> week or so.
>>
>>
>>
>> cu,
>>
>>
>>
>>
>>
>> On Sat, May 2, 2009 at 11:42 AM, Tyler Laing
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>>
>>
>> wrote:
>> > Hmm, just noticed another issue. I had to make the change
>> from
>> INT_MAX to
>> > LONG_MAX because the outputs for the specific number had
>> an 'L'
>> on the end
>> > of them, where the number exceeds INT_MAX, and so a 'L' is
>> appended to the
>> > end of the outputs from oct and hex, which causes a lot of
>> fails, as when
>> > Python converts the numbers to hex/int, there is no 'L'
>> on the
>> end of that
>> > value.
>> >
>> > Basically this line:
>> >
>> > self.assertEquals (hex (c), str(hex (0xCC00CC11))
>> >
>> > fails with this message:
>> >
>> >
>>
>> ======================================================================
>> > FAIL: ColorTypeTest.test_webstyle
>> >
>>
>> ----------------------------------------------------------------------
>> > Traceback (most recent call last):
>> > File
>>
>> "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
>> line
>> > 465, in test_webstyle
>> > self.assertEquals (hex (c), hex (0xCC00CC11))
>> > AssertionError: '0xcc00cc11L' != '0xcc00cc11'
>> >
>> > My fix, which I do not like, at all, is:
>> >
>> > self.assertEquals (str(hex (c))[:-1], str(hex (0xCC00CCFF)))
>> >
>> > I don't know if this would behave differently on a 32 bit
>> platform.
>> >
>> > Here is the patch.diff.
>> >
>> > -Tyler
>> >
>> > On Fri, May 1, 2009 at 6:23 PM, René Dudfield
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>> wrote:
>> >>
>> >> Hi,
>> >>
>> >> cool, nice one.
>> >>
>> >> make your change to the file/files, make sure it
>> compiles and
>> passes
>> >> the tests...
>> >>
>> >> Then run: svn diff > patch.diff
>> >>
>> >> Then send the patch to the mailing list... or upload it
>> somewhere, and
>> >> send a link to it if it's really big... or just paste
>> it into the
>> >> email if it's tiny.
>> >>
>> >> cu,
>> >>
>> >>
>> >>
>> >>
>> >> On Sat, May 2, 2009 at 11:21 AM, Tyler Laing
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>> wrote:
>> >> > Rene,
>> >> >
>> >> > Okay, so I've got a fix. You have to prefix color->r with
>> (long), and
>> >> > then
>> >> > for hex and oct functions, you need to change INT_MAX to
>> LONG_MAX for a
>> >> > 64
>> >> > bit platform.
>> >> >
>> >> > How do I make a patch for submission?
>> >> >
>> >> > -Tyler
>> >> >
>> >> > On Fri, May 1, 2009 at 5:33 PM, Tyler Laing
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>> wrote:
>> >> >>
>> >> >> Rene,
>> >> >>
>> >> >> You are right. I isolated the specific issue, and
>> here's a
>> sample .c
>> >> >> file
>> >> >> that shows the error on the 64 bit platform. When I get
>> something that
>> >> >> works
>> >> >> on the the test file, I'll try it on the actual
>> pygame code
>> and see how
>> >> >> the
>> >> >> test performs.
>> >> >>
>> >> >> -Tyler
>> >> >>
>> >> >> On Fri, May 1, 2009 at 4:30 PM, Tyler Laing
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>>
>>
>> >> >> wrote:
>> >> >>>
>> >> >>> I'll try that then. For reference, I am using an AMD
>> Athlon
>> 64 X2
>> >> >>> 5200+
>> >> >>> processor. What would be the proper way to make it
>> endian safe?
>> >> >>>
>> >> >>> Change the unsigned long tmp to unsigned int tmp?
>> >> >>>
>> >> >>> -Tyler
>> >> >>>
>> >> >>> On Fri, May 1, 2009 at 4:24 PM, René Dudfield
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>>
>>
>> >> >>> wrote:
>> >> >>>>
>> >> >>>> hi,
>> >> >>>>
>> >> >>>> there's some parts like this...
>> >> >>>>
>> >> >>>> static PyObject*
>> >> >>>> _color_float (PyColor *color)
>> >> >>>> {
>> >> >>>> unsigned long tmp = (color->r << 24) + (color->g
>> << 16) +
>> >> >>>> (color->b
>> >> >>>> << 8) +
>> >> >>>> color->a;
>> >> >>>> return PyFloat_FromDouble ((double) tmp);
>> >> >>>> }
>> >> >>>>
>> >> >>>> this code isn't endian or 64bit safe... since it is
>> using bit
>> >> >>>> shifting
>> >> >>>> for packing. On different platforms, this produces
>> different
>> >> >>>> outputs.
>> >> >>>>
>> >> >>>> I think it has to convert into the same 32bit unsigned
>> int, and then
>> >> >>>> return that.
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>> On Sat, May 2, 2009 at 8:54 AM, Tyler Laing
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>>
>>
>>
>> >> >>>> wrote:
>> >> >>>> > Taking a look at color.c, I believe the bug may
>> actually
>> rest in
>> >> >>>> > the
>> >> >>>> >
>>
>> Py<type>_FromUnsignedLong/Py<type>_FromDouble/Py<type>_FromString
>> >> >>>> > functions
>> >> >>>> > provided by the Python libs. There is no logical or
>> numerical
>> >> >>>> > reason
>> >> >>>> > why,
>> >> >>>> > from the numbers we have, we would get those
>> values with
>> those
>> >> >>>> > operations.
>> >> >>>> > The tests beforehand affirm that the r, g, b, and a
>> variables all
>> >> >>>> > the
>> >> >>>> > proper
>> >> >>>> > values, it just happens to be the one step in the
>> code. I'll
>> >> >>>> > examine
>> >> >>>> > further.
>> >> >>>> >
>> >> >>>> > -Tyler
>> >> >>>> >
>> >> >>>> > On Fri, May 1, 2009 at 3:28 PM, René Dudfield
>> <[email protected] <mailto:[email protected]>
>> <mailto:[email protected] <mailto:[email protected]>>>
>>
>>
>> >> >>>> > wrote:
>> >> >>>> >>
>> >> >>>> >> hi,
>> >> >>>> >>
>> >> >>>> >> Below are the failing unittests for Color on
>> 64bit ubuntu.
>> >> >>>> >>
>> >> >>>> >>
>> >> >>>> >>
>> >> >>>> >>
>> >> >>>> >>
>> >> >>>> >>
>> >> >>>> >>
>>
>> ======================================================================
>> >> >>>> >> > FAIL: ColorTypeTest.test_float
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ----------------------------------------------------------------------
>> >> >>>> >> > Traceback (most recent call last):
>> >> >>>> >> > File
>> >> >>>> >> >
>> "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
>> >> >>>> >> > line
>> >> >>>> >> > 412, in test_float
>> >> >>>> >> > self.assertEquals (float (c), float
>> (0xCC00CC00))
>> >> >>>> >> > AssertionError: 1.844674407283719e+19 !=
>> 3422604288.0
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ======================================================================
>> >> >>>> >> > FAIL: ColorTypeTest.test_hex
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ----------------------------------------------------------------------
>> >> >>>> >> > Traceback (most recent call last):
>> >> >>>> >> > File
>> >> >>>> >> >
>> "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
>> >> >>>> >> > line
>> >> >>>> >> > 442, in test_hex
>> >> >>>> >> > self.assertEquals (hex (c), hex (0xCC00CC00))
>> >> >>>> >> > AssertionError: '0xffffffffcc00cc00L' !=
>> '0xcc00cc00'
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ======================================================================
>> >> >>>> >> > FAIL: ColorTypeTest.test_int
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ----------------------------------------------------------------------
>> >> >>>> >> > Traceback (most recent call last):
>> >> >>>> >> > File
>> >> >>>> >> >
>> "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
>> >> >>>> >> > line
>> >> >>>> >> > 494, in test_int
>> >> >>>> >> > self.assertEquals (int (c), int (0xCC00CC00))
>> >> >>>> >> > AssertionError: 18446744072837188608L !=
>> 3422604288
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ======================================================================
>> >> >>>> >> > FAIL: ColorTypeTest.test_long
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ----------------------------------------------------------------------
>> >> >>>> >> > Traceback (most recent call last):
>> >> >>>> >> > File
>> >> >>>> >> >
>> "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
>> >> >>>> >> > line
>> >> >>>> >> > 511, in test_long
>> >> >>>> >> > self.assertEquals (long (c), long
>> (0xCC00CC00))
>> >> >>>> >> > AssertionError: 18446744072837188608L !=
>> 3422604288L
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ======================================================================
>> >> >>>> >> > FAIL: ColorTypeTest.test_oct
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ----------------------------------------------------------------------
>> >> >>>> >> > Traceback (most recent call last):
>> >> >>>> >> > File
>> >> >>>> >> >
>> "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
>> >> >>>> >> > line
>> >> >>>> >> > 427, in test_oct
>> >> >>>> >> > self.assertEquals (oct (c), oct (0xCC00CC00))
>> >> >>>> >> > AssertionError: '01777777777771400146000L' !=
>> '031400146000'
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ======================================================================
>> >> >>>> >> > FAIL: ColorTypeTest.test_webstyle
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >> >
>>
>> ----------------------------------------------------------------------
>> >> >>>> >> > Traceback (most recent call last):
>> >> >>>> >> > File
>> >> >>>> >> >
>> "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
>> >> >>>> >> > line
>> >> >>>> >> > 458, in test_webstyle
>> >> >>>> >> > self.assertEquals (hex (c), hex (0xCC00CC11))
>> >> >>>> >> > AssertionError: '0xffffffffcc00cc11L' !=
>> '0xcc00cc11'
>> >> >>>> >> >
>> >> >>>> >> >
>> >> >>>> >
>>
>>
>>
>
--
Visit my blog at http://oddco.ca/zeroth/zblog
Index: src/color.c
===================================================================
--- src/color.c (revision 2051)
+++ src/color.c (working copy)
@@ -1407,7 +1407,7 @@
static PyObject*
_color_int (PyColor *color)
{
- unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+ unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
color->a;
#if !PY3
if (tmp < INT_MAX)
@@ -1422,7 +1422,7 @@
static PyObject*
_color_long (PyColor *color)
{
- unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+ unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
color->a;
return PyLong_FromUnsignedLong (tmp);
}
@@ -1433,7 +1433,7 @@
static PyObject*
_color_float (PyColor *color)
{
- unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+ unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
color->a;
return PyFloat_FromDouble ((double) tmp);
}
@@ -1446,9 +1446,9 @@
_color_oct (PyColor *color)
{
char buf[100];
- unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+ unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
color->a;
- if (tmp < INT_MAX)
+ if (tmp < LONG_MAX)
PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
else
PyOS_snprintf (buf, sizeof (buf), "0%loL", tmp);
@@ -1462,9 +1462,9 @@
_color_hex (PyColor *color)
{
char buf[100];
- unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+ unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
color->a;
- if (tmp < INT_MAX)
+ if (tmp < LONG_MAX)
PyOS_snprintf (buf, sizeof (buf), "0x%lx", tmp);
else
{
@@ -1475,7 +1475,7 @@
PyOS_snprintf (buf, sizeof (buf), "0x%lXL", tmp);
#endif
}
- return Text_FromUTF8 (buf);
+ return PyString_FromString (buf);
}
#endif
