On Thu, Oct 27 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Add a "const" to the "struct string_list *" passed to >> for_each_string_list(). >> >> This is arguably abuse of the type system, as the >> "string_list_each_func_t fn" take a "struct string_list_item *", >> i.e. not one with a "const", and those functions *can* modify those >> items. >> >> But as we'll see in a subsequent commit we have other such iteration >> functions that could benefit from a "const", i.e. to declare that >> we're not altering the list itself, even though we might be calling >> functions that alter its values. > > The callback functions are allowed to (by taking a non-const > pointer) modify the items, but are there ones that actually modify > them? Tree-wide that's: 11 files changed, 18 insertions(+), 18 deletions(-) I.e. a bunch of changes like: -static int get_notes_refs(struct string_list_item *item, void *arg) +static int get_notes_refs(const struct string_list_item *item, void *arg) It turns out there's a grand total of one user of that: setup.c: In function ‘canonicalize_ceiling_entry’: setup.c:1102:30: error: assignment of member ‘string’ in read-only object 1102 | item->string = real_path; | ^ But note that that's for the "filter" variant. In any case using the same function pointer type in eb5f0c7a616 (string_list: add a new function, filter_string_list(), 2012-09-12) for both was probably a mistake. But still, I think it's best not to do anything about *that*. I.e. it makes sense for such an interface to say that the iterator helper takes your const list, i.e. unlike filter_string_list() it's not expected to be changing the list itself. But you as as the caller are then free to change list items you're given.
