[OPEN-ILS-DEV] C nits: object.c (Part 3)

[Scott McKellar]

The function _jsonObjectShifIndex is a near-duplicate of the
function _jsonObjectShiftIndex.  Unlike the latter, it is not
declared in object.h, nor referenced elsewhere in object.c.

I suspect that the former function is dead code, the result of an
editing accident, and should be eliminated entirely.

Both function names begin with an underscore followed by a lower case
letter.  Such names are reserved for use as identifiers with file 
scope.  These functions have global scope.

Apart from a one-character difference in their names, these functions
differ only in that _jsonObjectShifIndex() verifies that its index
argument is non-negative.  Since the index argument is unsigned,
this test is unnecessary.

When called, these functions assume that the jsonObject's
payload is a list of jsonObjectNodes.  They should verify this 
assumpton by checking the object type.  If the assumption is wrong, 
the results will be unpleasant.

They traverse the list and decrement the index for any node whose
index is greater than or equal to a specified threshold.  Depending
on the initial state of the list, this procedure could result in
duplicate index values.  I suspect that such duplication is not a
good thing, but I don't know what the intended rules are.

Finally, the function decrements the size member of the original
jsonObject.  The decrement confused me at first, because we aren't
removing any nodes from the list.  Eventually I realized that the
size member is not intended to represent the number of nodes in the
list, but the number of entries in a simulated and potentially
sparse array.  It should be one greater than the index of the last
node.  The size member is one-based, while the indexes are zero-based.
Have I got that right?

If I'm right about the size member, the jsonObjectRemoveIndex function 
doesn't maintain it correctly.

Suppose the list contains the index values 1, 7, and 15.  Presumably 
the size member contains the value 16.  Then we call 
jsonObjectRemoveIndex() to remove 15.  This function calls
jsonObjectShiftIndex(), which decrements the size member, which now
contains the value 15.  However if the size member is supposed to be
one greater than the index of the last node, it should contain the
value 8, not 15.  Arguably the value 15 represents the ghostly
persistence of nodes 8 through 14, which don't really exist, and 
might never have existed if we hadn't created 15 at some point.  I
don't know if there's a need to represent such trailing null entries.

If jsonObjectRemoveIndex() is the only function that calls
jsonObjectShift(), then the latter will not create duplicate index
values, because we will have removed one of the nodes that would have
been a duplicate.  However in that case jsonObjectIndex() should be 
declared static, and its prototype should be removed from the header.

In addition, there is a needless inefficiency here.  Instead of
passing a pointer to the original jsonObject, we should pass a 
pointer to the tail end of the list.  That way jsonObjectShiftIndex()
wouldn't have to start all over from the beginning of the list.

Scott McKellar
http://home.swbell.net/mck9/aargh/