| Message ID | patch-04.10-40b3cc9b8d4-20221026T151328Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config API: make "multi" safe, fix numerous segfaults | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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?
On Thu, Oct 27 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
diff --git a/string-list.c b/string-list.c index 549fc416d68..d8957466d25 100644 --- a/string-list.c +++ b/string-list.c @@ -129,7 +129,7 @@ void string_list_remove_duplicates(struct string_list *list, int free_util) } } -int for_each_string_list(struct string_list *list, +int for_each_string_list(const struct string_list *list, string_list_each_func_t fn, void *cb_data) { int i, ret = 0; diff --git a/string-list.h b/string-list.h index c7b0d5d0008..7153cb79154 100644 --- a/string-list.h +++ b/string-list.h @@ -138,7 +138,7 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c * Apply `func` to each item. If `func` returns nonzero, the * iteration aborts and the return value is propagated. */ -int for_each_string_list(struct string_list *list, +int for_each_string_list(const struct string_list *list, string_list_each_func_t func, void *cb_data); /**
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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- string-list.c | 2 +- string-list.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)