When compiling the newest 3.x trunk I've got a compiler warning that
get_ulong is defined but never used.

I moved the function near the only place where it is used (disabled code).

Now I have no more warnings.

I don't know if it's of any use.

Thanks for the patch!  Certainly it's desirable to get rid
of this warning, especially as in this case the warning indicates
a genuine problem:  namely that there's unused code floating around. 

I think it would not be unreasonable to simply remove the code
that's #ifdef'd away.  It was there for backwards compatibility,
and should no longer be needed in 3.0 and 3.1.

Hi Mark,

I've removed all overflow masking from _struct.c. 

Running the regression test test_struct failed for test_issue4228 wich I
think have to be removed too because it tests the deprecated feature.

I've tested the constraints with some values and got the expected
overflow errors.

So here's the new patch without overflow masking and deleted test_issue4228.

The patch is just tested on windows. So when I get home I'll give it a
try on Linux.

The latest patch looks good to me, and results in a much cleaner looking 
_struct.c.  Thank you!

One worry: the issue 4228 discussion suggests that the zipfile module 
still relies on the deprecated wrapping.  I think this needs to be 
investigated (and fixed if necessary) before this patch can be applied.

I think you're right about issue 4228:  we should go ahead and
clean up the struct module anyway.  As far as I can tell, only
the Python 2.5 zipfile module is using the deprecated behaviour.

The _struct.c portion of your patch looks fine.  I think there's
still some work to do on the test_struct file:  for example,
removing references to PY_STRUCT_OVERFLOW_MASKING.

I'd also like to remove the deprecated float handling, but it's
probably better to do that in a separate patch once this patch is in.

Changing title to better reflect the current patch.

I agree with you. A separate patch will do better.

In the next days I'm able to provide a new patch with the test_struct
cleanup.

Currently all tests succeeded on Windows and Linux. So I think no other
module relies on this feature explicitly.

I've already checked the documentation, but there are no references to
overflow masking. So there's nothing to do for now.

Ok, heres the patch again.

Passed regression tests.

Should the version number increase to 0.3? Maybe to reflect there are no
more deprecated features. It should then take place after FLOAT_COERCE
cleanup.

I'm currently trying to figure out whats about _PY_STRUCT_RANGE_CHECKING
and why it is hardcoded constant in _struct.c.

Thanks!  Committed your patch in r70497.

I think _PY_STRUCT_RANGE_CHECKING can also be removed from the module and 
the tests (treated as though it's 1 throughout).  In theory there could be 
people using it, but it's not documented and the leading underscore in the 
name clearly indicates that its supposed to be private, so I think it's 
safe to remove.  It looks like it was there just to enable some extra 
tests (tests that were broken with earlier versions of the struct module 
thanks to struct bugs).

I agree that we should bump the version number once we're done here.

I really appreciate this work:  the struct module has been in need of 
cleanup for a while.  Thank you again.

I removed definition of _PY_STRUCT_RANGE_CHECKING.
 
The test_standard_integers now expects struct.errors when there are out
of range values (should have been part of my last patch). So
test_1229380 is now obsolete.

Thanks for the patch.  A couple of questions and comments:

(1) at line 300-ish of test_struct, should (struct.error, TypeError) be 
(struct.error, OverflowError)?  I don't think out-of-range values should 
be raising TypeError.  If they are, perhaps we should change that.

(2) It looks like the deprecated_err function isn't needed any more 
(yay!);  let's remove it.

(3) I'd prefer to keep the test_1229380 bit, but just replace the 
deprecated_err with an assertRaises, just like you already did further 
up.  As far as I can see those tests aren't entirely duplicated by 
others, and one can never have too many tests...

I've messed with your patch a bit more.  :)  Here's the latest version.

Apart from the things I mentioned earlier, test_struct was failing on 
64-bit machines with your original patch;  I think that's fixed now.  I 
also updated the docs.

Does this version look okay to you?

Still the float coercion to get rid of.  I'd also like to replace the 
various struct.error exceptions with something more appropriate (e.g., 
OverflowError for out-of-range values, TypeError for floats passed to 
integer formats), but that probably needs a short python-dev discussion 
first.

Oops.  Out-of-date version of the diff.  Here's the right one.

Yes you're right. The TypeError should be an OverflowError. It was just
the copy and paste thing. 

Hm, I also wondering why struct.error is used. But someone already
wanted to change this.

The patch looks fine.

Do you want to go ahead with the float coercion thing? Maybe I'll find
some time the next week to care about. I think there's no hurry.

Thanks again, Andreas.  Applied in r70688.

There's no particular hurry on removing the float coercion, except that 
I'd like to get it in before the first 3.1 beta.

Doesn't took much time. Even nothing happend.
PyLong_AsLong trys to convert to int bevor raising an Exception.
 
I'm not sure if struct.pack('l', x) should raise an Exception when a
float is given. In case of string there is one.

I first did PyLong_Check the parameter and raised an StructError. But I
removed this from the patch.

I leaved the documentation untouched. Nobody will notice the changes.

I increased the version number. Whether we overwork the Errors prior
3.1b or not we could leave it at 0.3.

Hi,

could you have a look at cleanup_float_coerce_patch.diff.

The _struct.c part of the patch looks good.

For the tests, there should be some tests to check that
attempting to pack a float with an integer format gives
either TypeError or struct.error.

Thanks!

Hi Mark,

currently there will be no struct.error neither TypeErorr because
PyLong_AsLong floors a given float (see my msg84620).

The Question is: should I test for long explicitly and raise an error if
a different type is given? 
In this case a test is needed, I give in.

I'm not sure what's the right style here.

Hmm.  In 3.0 and 2.7, I get:

>>> from struct import pack
>>> pack('L', 123.0)
sys:1: DeprecationWarning: integer argument expected, got float
b'{\x00\x00\x00'

So it looks like we already changed py3k to get rid of the 
DeprecationWarning.

I think the idea was that eventually *only* integers would be accepted 
for the integer format codes.  So pack('L', 123.0) should ideally raise 
TypeError.  If we're not going to do that, we should at least put the 
DeprecationWarning back in, but it seems better to actually go ahead 
with the deprecation.

Here's an updated version of your patch:  all I've done is fix up
get_(u)long, get_(u)longlong and get_pylong so that they only accept 
integers.

Committed part 3 in r71755.  Thank you!

Is there anything else you think we need to fix up here?

That should be r71754, of course.

I think we're done here.

There's only the struct.error to be replaced by OverflowError or
TypeError. Do you start the discussion on python-dev? I don't know how to.

Yes, there should probably be a python-dev discussion.  I'll
add it to my list of things to do, if you like!

And I still have to deal with the original compiler warning
that started it all in trunk...

The trunk warning was squashed in r73004.