On 06/01/2016 01:12 AM, Eric Sunshine wrote: > On Tue, May 31, 2016 at 3:59 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: >> On 05/31/2016 07:29 AM, Eric Sunshine wrote: >>> On Mon, May 30, 2016 at 3:55 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: >>>> +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. > >> 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. >>> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html