[cairo] [cairo-commit] 3 commits - src/cairo-array.c src/cairoint.h src/cairo-types-private.h

[Bill Spitzak]

This code in the comment makes no sense to me:

>>>> +    /* We allow an index of 0 for the no-elements case.
>>>> +     * This makes for cleaner calling code which will often look like:
>>>> +     *
>>>> +     *    elements = _cairo_array_index_const (array, num_elements);
>>>> +     *         for (i=0; i < num_elements; i++) {
>>>> +     *        ... read elements[i] here ...
>>>> +     *    }
>>>> +     *
>>>> +     * which in the num_elements==0 case gets the NULL pointer here,
>>>> +     * but never dereferences it.
>>>> +     */

If we assume the size of the array items is 1, the first line is the 
same as elements = array+num_elements. The next line of the code seems 
to think this means that elements[num_elements-1] will not go off the 
end of the array. But this is actually array[num_elements*2-1], and we 
only know array is num_elements long or more.

My best guess is that the last line of the code in the comment is 
supposed to be "read array[i] here".

But this still makes no sense. If the purpose is to get the assert to 
throw before the code will index off the end of the array, this will 
only work if you pass num_elements-1 to the call. This means that if the 
loop is given num_elements == 0, it will actually pass -1 to the call 
(or ~0 if this is unsigned).

I really see no purpose in the way the code is written. Either it should 
always throw an assert for a zero-length array, or it should not throw 
an assert for indexing one off the end of a non-zero-length array.