On Thu, Oct 27 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Refactor a "do I have an element like this?" pattern added in [1] and >> [2] to use unsorted_string_list_has_string() instead of a >> for_each_string_list_item() loop. > > In the longer term, I am not sure if we want to keep such code that > uses string-list as a "database to be looked up with the string as > the key". I am not sure it is worth our review bandwidth to change > a for-each-string-list that terminates early to its shorthand > unsorted_string_list_has_string(). Surely each such conversation > would allow us to lose 4 to 5 lines, but longer term we should be > discuraging the use of unsorted_string_list_has_string() in the > first place. I haven't benchmarked, but I'd think on modern computers O(n) for such short lists would be more performance due to cache locality, i.e. not worth pre-sorting it, or making it a hash table. But for such small amounts of data I'd think it would be fine either way. As to the change, I'm fine with leaving this out. The reason it's in here is because this series came out as a reply to Stolee's earlier RFC. I think it's a fair summary to say that the reason we started talking about this at all is because the topic at hand was how to make this exact code in builtin/gc.c safer and more idiomatic. I.e. see: https://lore.kernel.org/git/e06cb4df081bc2222731f9185a22ed7ad67e3814.1664287711.git.gitgitgadget@xxxxxxxxx/ And my earlier summary of that, the very beginning showing the API forms under discussion: https://lore.kernel.org/git/220928.868rm3w9d4.gmgdl@xxxxxxxxxxxxxxxxxxx/ So Re this & your "it might be good, but why does it have to be done here now?" reply to the CL: Yeah I can eject some of this, but having a series (and a predecessor RFC) whose main reason for existing is making this API safer & nicer seems incomplete unless we're also converting callers to use those supposedly nicer patterns. In general I think this sort of change is exactly the sort of thing you'd want in such a series. A test of a good API isn't just that it looks or acts nicely in isolation, but that it's easily combined with other things you might expect to use with it. Hence this 04-06/10. I.e. the original contention in the RFC was that we had to return a dummy string list to make these sort of patterns safer/nicer/idiomatic. I think these patches serve as a convincing counter-argument to that.
