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]> 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 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]> 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]> 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]>
> 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]>
> 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]>
> >> >> 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]>
> >> >>> 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]>
> >> >>>> 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]>
> >> >>>> > 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
> >> >>>> >
> >> >>>
> >> >>>
> >> >>>
> >> >>> --
> >> >>> Visit my blog at http://oddco.ca/zeroth/zblog
> >> >>
> >> >>
> >> >>
> >> >> --
> >> >> Visit my blog at http://oddco.ca/zeroth/zblog
> >> >
> >> >
> >> >
> >> > --
> >> > Visit my blog at http://oddco.ca/zeroth/zblog
> >> >
> >
> >
> >
> > --
> > Visit my blog at http://oddco.ca/zeroth/zblog
> >
>
--
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 = ((long)color->r << 24) + ((long)color->g << 16) + ((long)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 = ((long)color->r << 24) + ((long)color->g << 16) + ((long)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 = ((long)color->r << 24) + ((long)color->g << 16) + ((long)color->b << 8) +
color->a;
return PyFloat_FromDouble ((double) tmp);
}
@@ -1446,12 +1446,12 @@
_color_oct (PyColor *color)
{
char buf[100];
- unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+ 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);
+ PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
return PyString_FromString (buf);
}
@@ -1462,17 +1462,17 @@
_color_hex (PyColor *color)
{
char buf[100];
- unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+ 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), "0x%lx", tmp);
else
{
#if PY_VERSION_HEX >= 0x02050000
- PyOS_snprintf (buf, sizeof (buf), "0x%lxL", tmp);
+ PyOS_snprintf (buf, sizeof (buf), "0x%lx", tmp);
#else
/* <= 2.4 uses capitalised hex chars. */
- PyOS_snprintf (buf, sizeof (buf), "0x%lXL", tmp);
+ PyOS_snprintf (buf, sizeof (buf), "0x%lX", tmp);
#endif
}
return Text_FromUTF8 (buf);
