[Python-Dev] remaining issues from Klocwork static analysis

["Martin v. Löwis"]

Neal Norwitz wrote:
>> Not quite sure what it is complaining about, but
>>
>>         else if (PyTuple_Check(closure)) {
>>                 Py_XINCREF(closure);
>>         }
>>
>> looks indeed suspicious: Why do we check for NULL (XINCREF) when
>> we know closure can't be NULL (Tuple_Check). Drop the X, and see
>> if the warning goes away
> 
> Yes, I definitely think dropping the X would make the warning go away.
> Do we want to check for a NULL pointer and raise an exception?  The
> docs don't address the issue, so I think if we added a check, ie:  if
> (closure && PyTuple_Check(closure)) and got rid of the X that would be
> fine as well.

The docs do address the issue:

\var{closure} must be \var{Py_None} or a tuple of cell objects.

It doesn't allow for NULL, and None indicates that the closure
should become NULL. The only caller of it in the core will never
pass NULL.

If you want to check that this is not NULL on the grounds that
somebody may call it incorrectly, then you should also check that
op is not NULL, because somebody may call it incorrectly.

> The first line of data is (fro unicodedata_db.h) is:
> 
> static unsigned int decomp_data[] = {
>    0, 257, 32, 514, 32, 776, 259, 97, 514, 32, 772, 259, 50, 259, 51, 514,

Read this as
0: sentinel
257 = 256 | 1: length 1, prefix 1
32: U+0020
514 = 512 | 2: length 2, prefix 2
32: U+0020
776: U+308
...

> Is that correct?  If so, would it be correct to add:
> 
>    unsigned short prefix_index = decomp_data[index] & 255;
>    assert(prefix_index < (sizeof(decomp_prefix)/sizeof(*decomp_prefix)));

Yes, that would be correct.

Regards,
Martin