Patches item #1530738, was opened at 2006-07-28 20:47 Message generated for change (Comment added) made by nnorwitz You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1530738&group_id=5470
Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: Core (C code) Group: Python 2.5 Status: Open Resolution: None Priority: 7 Submitted By: Nick Coghlan (ncoghlan) Assigned to: Nobody/Anonymous (nobody) Summary: Fix __index__() clipping really big numbers Initial Comment: Patch attached (index_overflow.diff) that causes __index__() to raise OverflowError for really big numbers instead of silently clipping them. The approach in the patch is a "minimal fix" that is as ugly as hell (3 different error return codes!), so I'm going to try for a cleaner version that changes nb_index to return a PyObject* (then the client code can decide whether to convert to Py_ssize_t or not, and whether to clip or raise OverflowError when doing so). ---------------------------------------------------------------------- >Comment By: Neal Norwitz (nnorwitz) Date: 2006-07-29 11:20 Message: Logged In: YES user_id=33168 The description is no longer valid, this is not a minimal patch. I haven't thought about the actual design of the patch, but I have a bunch of issues. In no particular, order: * tests should not use the assert statement, but self.assert* * There are a bunch of formatting changes (one place with a couple of extra blank lines, other places where tabs became spaces or vica versa) * I think there might be behaviour changes wrt checking for sq_ass_item and sq_get_item and raising type_error()s. I didn't think about this hard, but just from a quick review there was a red flag. * PyNumber_Index(o, is_index) is a bogus naming. Either the API should change or is_index should change. It doesn't make sense to me that you would call an API to get the index that is not an index. * int_index became int_int, same with long, but not in other places. I don't understand the name change. I need to think more about the structure of this patch. There's something I don't like, but I'm not sure what it is without further review. More eyes would definitely help. ---------------------------------------------------------------------- Comment By: Nick Coghlan (ncoghlan) Date: 2006-07-29 09:43 Message: Logged In: YES user_id=1038590 Revised the patch to further reduce code duplication in the implementation, and to reduce the divergence from PEP 357. The functions in the C API now have the names PyNumber_Index (raises IndexError), PyNumber_AsSsize_t (raises OverflowError) and PyNumber_AsClippedSsize_t (clamps to PY_SSIZE_T_MIN/MAX), and are no longer exposed through the operator module. Python code should either use __index__() directly (if not constrained to concrete containers) or else use operator.index(). ---------------------------------------------------------------------- Comment By: Nick Coghlan (ncoghlan) Date: 2006-07-29 06:19 Message: Logged In: YES user_id=1038590 Attaching the patch that approaches the problem from the ground up by redesigning the PEP 357 C API to meet the needs of the actual use cases in the standard library. ---------------------------------------------------------------------- Comment By: Tim Peters (tim_one) Date: 2006-07-28 21:05 Message: Logged In: YES user_id=31435 Since I don't think this is a sane approach, I'm not going to spend time reviewing it :-) Suggest working out what's /wanted/ on python-dev first, including beefing up PEP 357 so it spells out the revised intents. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1530738&group_id=5470 _______________________________________________ Patches mailing list Patches@python.org http://mail.python.org/mailman/listinfo/patches
