[MERGE] GraphIndex bisection

Andrew Bennetts andrew at canonical.com
Mon Oct 8 07:42:56 BST 2007


bb:comment

This isn't a proper review, just a comment on something that jumped out at me
while skimming:

Robert Collins wrote:
> This adds the facility for partial index reads to the GraphIndex class. 
[...]
> +
> +def bisect_multi_bytes(content_lookup, size, keys):
> +    """Perform bisection lookups for keys using byte based addressing.
> +    
> +    The keys are looked up via the content_lookup routine. The content_lookup
> +    routine gives bisect_multi_bytes information about where to keep looking up
> +    to find the data for the key, and bisect_multi_bytes feeds this back into
> +    the lookup function until the search is complete. The search is complete
> +    when the list of keys which have returned something other than -1 or +1 is
> +    empty. Keys which are not found are not returned to the caller.
> +
> +    :param content_lookup: A callable that takes a list of (offset, key) pairs
> +        and returns a list of result tuples ((offset, key), result). Each
> +        result can be one of:
> +          -1: The key comes earlier in the content.
> +          False: The key is not present in the content.
> +          +1: The key comes later in the content.
> +          Any other value: A final result to return to the caller.
> +    :param size: The length of the content.
> +    :param keys: The keys to bisect for.
> +    :return: An iterator of the results.

The -1/False/+1/result interface seems a bit distasteful to me.  Partly this is
because it “result” sounds like it may be any object, but obviously you've
precluded -1, False and +1 as possible values.  Also partly because mixing
bools and ints seems likely to cause confusion.  And just generally because
making one value do triple-duty seems like a kludge.

Probably just clarifying what 'result' can (or cannot) be would be enough to
make me happy.  There are four distinct states here, so in that respect this is
neater than trying to make a two-tuple or something out of it.  But the initial
impression is of “oh it can be one of a couple of different types legitimately,
or any other arbitrary object, that seems likely to cause trouble”.

-Andrew.
 



More information about the bazaar mailing list