StaticTuple_New handles size < 0 and size > 255 differently
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Bazaar |
Fix Released
|
Wishlist
|
Matt Nordhoff |
Bug Description
Lookie:
if (size < 0) {
return NULL;
}
if (size < 0 || size > 255) {
/* Too big or too small */
" takes from 0 to 255 items");
return NULL;
}
Not only is this redundant, but PyErr_BadIntern
ISTM StaticTuple_New should throw a ValueError, since you do StaticTuple_
FYI, the Python version of StaticTuple throws a ValueError when it's either <0 or >255. IMO, that should be a TypeError, same as StaticTuple_
Related branches
- John A Meinel: Approve
-
Diff: 67 lines (+13/-9)3 files modifiedbzrlib/_static_tuple_c.c (+8/-4)
bzrlib/_static_tuple_py.py (+3/-3)
bzrlib/tests/test__static_tuple.py (+2/-2)
description: | updated |
Changed in bzr: | |
importance: | Undecided → Wishlist |
status: | New → In Progress |
Changed in bzr: | |
assignee: | nobody → Matt Nordhoff (mnordhoff) |
status: | In Progress → Fix Released |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Matt Nordhoff wrote: alCall( ); (PyExc_ ValueError, "StaticTuple(...)" alCall( ) throws a New(length) , and StaticTuple_ new_constructor should throw a new_constructor . alCall( ); alCall( ); (PyExc_ ValueError, "StaticTuple(...)" (PyExc_ ValueError, "StaticTuple(...)" alCall( ) throws a
> Public bug reported:
>
> Lookie:
>
> if (size < 0) {
> PyErr_BadIntern
> return NULL;
> }
>
> if (size < 0 || size > 255) {
> /* Too big or too small */
> PyErr_SetString
> " takes from 0 to 255 items");
> return NULL;
> }
>
> Not only is this redundant, but PyErr_BadIntern
> TypeError, meaning two different exception classes are used.
>
> ISTM StaticTuple_New should throw a ValueError, since you do
> StaticTuple_
> TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
> StaticTuple_New handle throw the error).
>
> FYI, the Python version of StaticTuple throws a ValueError when it's
> either <0 or >255. IMO, that should be a TypeError, same as
> StaticTuple_
>
> ** Affects: bzr
> Importance: Undecided
> Status: New
>
> ** Description changed:
>
> Lookie:
>
> - if (size < 0) {
> - PyErr_BadIntern
> - return NULL;
> - }
> + if (size < 0) {
> + PyErr_BadIntern
> + return NULL;
> + }
>
> - if (size < 0 || size > 255) {
> - /* Too big or too small */
> - PyErr_SetString
> - " takes from 0 to 255 items");
> - return NULL;
> - }
> + if (size < 0 || size > 255) {
> + /* Too big or too small */
> + PyErr_SetString
> + " takes from 0 to 255 items");
> + return NULL;
> + }
>
> Not only is this redundant, but PyErr_BadIntern
> TypeError, meaning two different exception classes are used.
Actually, PyErr_BadIntern alCall raises a "SystemError": BadInternalCall (char *filename, int lineno) Format( PyExc_SystemErr or,
void
_PyErr_
{
PyErr_
...
My specific point was that passing size < 0 is a sign that something is
very wrong. Which is different from just passing a value that is too large.
The redundant "<0" is probably because I used to check for size < 1, and
just didn't remove that check when I started allowing size 0 StaticTuples.
> New(length) , and StaticTuple_ new_constructor should throw a new_constructor .
> ISTM StaticTuple_New should throw a ValueError, since you do
> StaticTuple_
> - TypeError, since you do StaticTuple(foo, bar, baz).
> + TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
> + StaticTuple_New handle throw the error).
>
> FYI, the Python version of StaticTuple throws a ValueError when it's
> either <0 or >255. IMO, that should be a TypeError, same as
> StaticTuple_
>
I'm happy to have them the same. Regardless of what we make this, it
isn't something you should ever be catching, so the specific exception
isn't particularly meaningful.
It is arguable that "TypeError" should be reserved for when the *type* unicode/ etc.)
of the argument passed is invalid. (Such as passing a dict rather than a
string/
Everything else would be considered a ...