Comment 1 for bug 457979

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Public bug reported:
>
> Lookie:
>
> if (size < 0) {
> PyErr_BadInternalCall();
> return NULL;
> }
>
> if (size < 0 || size > 255) {
> /* Too big or too small */
> PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
> " takes from 0 to 255 items");
> return NULL;
> }
>
> Not only is this redundant, but PyErr_BadInternalCall() throws a
> TypeError, meaning two different exception classes are used.
>
> ISTM StaticTuple_New should throw a ValueError, since you do
> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
> 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_new_constructor.
>
> ** Affects: bzr
> Importance: Undecided
> Status: New
>
> ** Description changed:
>
> Lookie:
>
> - if (size < 0) {
> - PyErr_BadInternalCall();
> - return NULL;
> - }
> + if (size < 0) {
> + PyErr_BadInternalCall();
> + return NULL;
> + }
>
> - if (size < 0 || size > 255) {
> - /* Too big or too small */
> - PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
> - " takes from 0 to 255 items");
> - return NULL;
> - }
> + if (size < 0 || size > 255) {
> + /* Too big or too small */
> + PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
> + " takes from 0 to 255 items");
> + return NULL;
> + }
>
> Not only is this redundant, but PyErr_BadInternalCall() throws a
> TypeError, meaning two different exception classes are used.

Actually, PyErr_BadInternalCall raises a "SystemError":
void
_PyErr_BadInternalCall(char *filename, int lineno)
{
 PyErr_Format(PyExc_SystemError,

...

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.

>
> ISTM StaticTuple_New should throw a ValueError, since you do
> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
> - 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_new_constructor.
>

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*
of the argument passed is invalid. (Such as passing a dict rather than a
string/unicode/etc.)

Everything else would be considered a ValueError. (Having a tuple that
holds only strings but is 266 entries long is closer to a ValueError
than a TypeError.)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrgdB8ACgkQJdeBCYSNAANIfwCfV0XlkfHbfFZ40MHOERphiscs
51UAmwegOc60Jv9tVTResb7LecruFEI7
=i+3B
-----END PGP SIGNATURE-----