On Fri, Dec 04, 2020 at 02:11:51PM -0500, Taylor Blau wrote: > > Yeah, I think that a full-blown iterator type is overkill for this > > purpose. Another possible approach could be a macro: > > > > #define for_each_oid_array_unique(arr, i) \ > > for (i = 0; (i) < (array)->nr; i = oid_array_for_each_unique((arr), i)) > > > > but I don't think that's making anything more clear than > > 'oid_array_for_each_unique' already is. So, I like the approach that you > > took here. > > Hmm. I take part of that back; it would simplify the caller in the eighth > patch, which first wants to sort the list before iterating it (which is > necessary, because we look at 'i = 0' before calling > oid_array_for_each_unique()). > > So this could look instead like: > > #define for_each_oid_array_unique(arr, i) \ > for (oid_array_sort(), i = 0; (i) < (array)->nr; i = oid_array_for_each_unique((arr), i)) > > Maybe a little too magical, who knows. That was one of the many variants I wrote on the way here. But yeah, I think it is better to avoid magic macros unless they are getting us a lot. In the case of hashmap, there is a lot of magic needed to convert the hashmap_entry field back into the parent struct pointer. But here, we can get away with a pretty natural looking for-loop, so I think it's worth keeping it simple. -Peff
