Re: [PATCH 09/13] refs: introduce an iterator interface

On 06/01/2016 01:12 AM, Eric Sunshine wrote:

On Tue, May 31, 2016 at 3:59 AM, Michael Haggerty [off-list ref] wrote:

quoted

On 05/31/2016 07:29 AM, Eric Sunshine wrote:

quoted

On Mon, May 30, 2016 at 3:55 AM, Michael Haggerty [off-list ref] wrote:

quoted

+struct ref_iterator *empty_ref_iterator_begin(void);
+
+/*
+ * Return true iff ref_iterator is an empty_ref_iterator.
+ */
+int is_empty_ref_iterator(struct ref_iterator *ref_iterator);

I can see that you used this function as an optimization or
convenience in overlay_ref_iterator_begin(), but do you expect it to
be generally useful otherwise? Is it worth publishing? Do you have
other use-cases in mind?

It is only "published" within the refs module, in refs/refs-internal.h.
This header file is not meant to be used by code outside of the refs module.

Ah, I forgot about that. In that case, it's probably less of an issue.

quoted

My thinking was that it might be useful to other reference backends. The
function is pretty safe for anybody to call, though I admit that it is
not very general.

I don't have a strong feeling either way. If nobody else chimes in, I'll
remove it from the header file as you suggested. We can always add it
back if somebody needs it.

I don't feel strongly about it either.

OK then, I'll leave it as-is.

quoted

quoted

Also, can you explain why the merge iterator doesn't also perform the
optimization/convenience of checking if one iterator is an empty
iterator?

That's because the merge iterator doesn't know what its select function
will do. For example, you could imagine an "intersect" select function
that only lets through references that were in *both* sub-iterators. In
that case, your suggested "optimization" would be incorrect.

Makes sense. Thanks for explaining. I wonder if this deserves a
comment somewhere in code or commit message to make the situation
clear to a future developer who might think it a good idea to promote
the "optimization" to the merge iterator.

Good idea. I'll add a comment.

[...]

Thanks,
Michael