StaticTuple_New handles size < 0 and size > 255 differently

-----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 ...

John A Meinel wrote:
> 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,
>
> ...

Oh, huh. Thanks for nothing,
<http://docs.python.org/c-api/exceptions.html#PyErr_BadInternalCall>.

> 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.

Hm, that makes sense.

> 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.

Oh, okay.

>> 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 t...