classification
Title: Remove deprecated features from struct module
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.1
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: andreas.schawo, mark.dickinson
Priority: normal Keywords: patch

Created on 2009-03-09 21:17 by andreas.schawo, last changed 2009-07-04 08:54 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
struct_overflow_patch.diff andreas.schawo, 2009-03-20 21:35
cleanup_range_check_patch.diff andreas.schawo, 2009-03-23 15:24
cleanup_range_check_patch2.diff mark.dickinson, 2009-03-28 16:09
cleanup_float_coerce_patch.diff andreas.schawo, 2009-03-30 19:15
cleanup_float_coerce_patch_v2.diff mark.dickinson, 2009-04-19 16:25
Messages (25)
msg83406 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-03-09 21:17
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.
msg83420 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-10 07:46
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.
msg83465 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-03-11 11:28
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.
msg83583 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-14 12:11
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.
msg83746 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-18 13:32
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.
msg83749 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-03-18 14:02
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.
msg83868 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-03-20 21:35
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.
msg83931 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-21 10:40
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.
msg84009 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-03-23 15:24
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.
msg84258 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-27 13:47
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...
msg84309 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-28 16:01
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.
msg84311 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-28 16:09
Oops.  Out-of-date version of the diff.  Here's the right one.
msg84411 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-03-29 15:38
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.
msg84415 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-03-29 17:02
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.
msg84620 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-03-30 19:15
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.
msg85985 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-04-15 10:02
Hi,

could you have a look at cleanup_float_coerce_patch.diff.
msg85997 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-04-15 15:57
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!
msg86037 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-04-16 17:46
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.
msg86174 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-04-19 15:30
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.
msg86176 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-04-19 16:25
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.
msg86178 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-04-19 20:42
Committed part 3 in r71755.  Thank you!

Is there anything else you think we need to fix up here?
msg86179 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-04-19 20:43
That should be r71754, of course.
msg86820 - (view) Author: Andreas Schawo (andreas.schawo) Date: 2009-04-29 19:37
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.
msg86821 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-04-29 19:39
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...
msg90108 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-04 08:54
The trunk warning was squashed in r73004.
History
Date User Action Args
2009-07-04 08:54:24mark.dickinsonsetstatus: open -> closed

messages: + msg90108
2009-04-29 19:39:51mark.dickinsonsetmessages: + msg86821
2009-04-29 19:37:50andreas.schawosetstatus: pending -> open

messages: + msg86820
2009-04-19 20:44:33mark.dickinsonsetstatus: open -> pending
2009-04-19 20:43:19mark.dickinsonsetstatus: pending -> open

messages: + msg86179
2009-04-19 20:42:44mark.dickinsonsetstatus: open -> pending
resolution: accepted
messages: + msg86178

stage: test needed -> resolved
2009-04-19 16:25:34mark.dickinsonsetfiles: + cleanup_float_coerce_patch_v2.diff

messages: + msg86176
2009-04-19 15:30:18mark.dickinsonsetmessages: + msg86174
2009-04-16 17:46:32andreas.schawosetmessages: + msg86037
2009-04-15 15:57:15mark.dickinsonsetstage: patch review -> test needed
2009-04-15 15:57:05mark.dickinsonsetmessages: + msg85997
2009-04-15 10:02:57andreas.schawosetmessages: + msg85985
2009-03-30 19:15:11andreas.schawosetfiles: + cleanup_float_coerce_patch.diff

messages: + msg84620
2009-03-29 17:02:06mark.dickinsonsetmessages: + msg84415
2009-03-29 15:38:37andreas.schawosetmessages: + msg84411
2009-03-28 16:24:50mark.dickinsonsetpriority: normal
stage: patch review
2009-03-28 16:09:57mark.dickinsonsetfiles: - cleanup_range_check_patch2.diff
2009-03-28 16:09:51mark.dickinsonsetfiles: + cleanup_range_check_patch2.diff

messages: + msg84311
2009-03-28 16:01:58mark.dickinsonsetfiles: + cleanup_range_check_patch2.diff

messages: + msg84309
2009-03-27 13:47:37mark.dickinsonsetmessages: + msg84258
2009-03-23 20:13:30mark.dickinsonsetassignee: mark.dickinson
2009-03-23 15:25:24andreas.schawosetfiles: - struct_overflow_patch.diff
2009-03-23 15:25:01andreas.schawosetfiles: + cleanup_range_check_patch.diff

messages: + msg84009
2009-03-21 10:40:39mark.dickinsonsetmessages: + msg83931
2009-03-20 21:35:10andreas.schawosetfiles: + struct_overflow_patch.diff

messages: + msg83868
2009-03-18 14:02:12andreas.schawosetmessages: + msg83749
2009-03-18 13:32:13mark.dickinsonsettype: compile error -> enhancement
messages: + msg83746
title: Compiler warning get_ulong is never used 3.x -> Remove deprecated features from struct module
2009-03-14 12:11:08mark.dickinsonsetmessages: + msg83583
2009-03-11 11:28:58andreas.schawosetfiles: - get_ulong_patch.diff
2009-03-11 11:28:48andreas.schawosetfiles: + struct_overflow_patch.diff

messages: + msg83465
2009-03-10 07:46:48mark.dickinsonsetnosy: + mark.dickinson
messages: + msg83420
2009-03-09 21:17:18andreas.schawocreate