On Thu, Jul 29, 2021 at 2:09 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Thu, Jul 29, 2021 at 12:37:52PM -0600, Elijah Newren wrote:
>
> > > Arguably, the existence of these function indirections is perhaps a sign
> > > that the strmap API should provide a version of the clear functions that
> > > takes "partial / not-partial" as a parameter.
> >
> > Are you suggesting a modification of str{map,intmap,set}_clear() to
> > take an extra parameter, or removing the
> > str{map,intmap,set}_partial_clear() functions and introducing new
> > functions that take a partial/not-partial parameter? I think you're
> > suggesting the latter, and that makes more sense to me...but I'm
> > drawing blanks trying to come up with a reasonable function name.
>
> It does seem a shame to add the "partial" parameter to strmap_clear(),
> just because most callers don't need it (so they end up with this
> inscrutable "0" parameter).
>
> What if there was a flags field? Then it could be combined with the
> free_values parameter. The result is kind of verbose in two ways:
>
> - now strset_clear(), etc, need a "flags" parameter, which they didn't
> before (and is just "0" most of the time!)
>
> - now "strmap_clear(foo, 1)" becomes "strmap_clear(foo, STRMAP_FREE_VALUES)".
> That's a lot longer, though arguably it's easier to understand since
> the boolean is explained.
>
> Having gone through the exercise, I am not sure it is actually making
> anything more readable (messy patch is below for reference).
Thanks for diving in. Since it's not clear if it's helping, I'll just
take your earlier suggestion to rename the "strmap_func" variable to
"strmap_clear_func" instead.
>
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 3e7ab1ca82..dfbdba53da 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -242,7 +242,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
> insert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);
> }
>
> - strset_clear(&dups);
> + strset_clear(&dups, 0);
> strbuf_release(&ident);
> strbuf_release(&oneline);
> }
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 7e6b3e1b14..0c960111d1 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -665,9 +665,10 @@ void partial_clear_dir_rename_count(struct strmap *dir_rename_count)
>
> strmap_for_each_entry(dir_rename_count, &iter, entry) {
> struct strintmap *counts = entry->value;
> - strintmap_clear(counts);
> + strintmap_clear(counts, 0);
> }
> - strmap_partial_clear(dir_rename_count, 1);
> + strmap_clear(dir_rename_count,
> + STRMAP_FREE_VALUES | STRMAP_PARTIAL_CLEAR);
> }
>
> static void cleanup_dir_rename_info(struct dir_rename_info *info,
> @@ -683,15 +684,15 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,
> return;
>
> /* idx_map */
> - strintmap_clear(&info->idx_map);
> + strintmap_clear(&info->idx_map, 0);
>
> /* dir_rename_guess */
> - strmap_clear(&info->dir_rename_guess, 1);
> + strmap_clear(&info->dir_rename_guess, STRMAP_FREE_VALUES);
>
> /* relevant_source_dirs */
> if (info->relevant_source_dirs &&
> info->relevant_source_dirs != dirs_removed) {
> - strintmap_clear(info->relevant_source_dirs);
> + strintmap_clear(info->relevant_source_dirs, 0);
> FREE_AND_NULL(info->relevant_source_dirs);
> }
>
> @@ -716,7 +717,7 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,
>
> if (!strintmap_get(dirs_removed, source_dir)) {
> string_list_append(&to_remove, source_dir);
> - strintmap_clear(counts);
> + strintmap_clear(counts, 0);
> continue;
> }
>
> @@ -1045,8 +1046,8 @@ static int find_basename_matches(struct diff_options *options,
> }
> }
>
> - strintmap_clear(&sources);
> - strintmap_clear(&dests);
> + strintmap_clear(&sources, 0);
> + strintmap_clear(&dests, 0);
>
> return renames;
> }
> @@ -1700,7 +1701,7 @@ void diffcore_rename_extended(struct diff_options *options,
> FREE_AND_NULL(rename_src);
> rename_src_nr = rename_src_alloc = 0;
> if (break_idx) {
> - strintmap_clear(break_idx);
> + strintmap_clear(break_idx, 0);
> FREE_AND_NULL(break_idx);
> }
> trace2_region_leave("diff", "write back to queue", options->repo);
> diff --git a/merge-ort.c b/merge-ort.c
> index 0fb942692a..0765e23577 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -532,15 +532,10 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
> {
> struct rename_info *renames = &opti->renames;
> int i;
> - void (*strmap_func)(struct strmap *, int) =
> - reinitialize ? strmap_partial_clear : strmap_clear;
> - void (*strintmap_func)(struct strintmap *) =
> - reinitialize ? strintmap_partial_clear : strintmap_clear;
> - void (*strset_func)(struct strset *) =
> - reinitialize ? strset_partial_clear : strset_clear;
> + unsigned flags = reinitialize ? STRMAP_PARTIAL_CLEAR : 0;
>
> if (opti->pool)
> - strmap_func(&opti->paths, 0);
> + strmap_clear(&opti->paths, flags);
> else {
> /*
> * We marked opti->paths with strdup_strings = 0, so that
> @@ -550,15 +545,15 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
> * to these strings, it is time to deallocate them.
> */
> free_strmap_strings(&opti->paths);
> - strmap_func(&opti->paths, 1);
> + strmap_clear(&opti->paths, flags | STRMAP_FREE_VALUES);
> }
>
> /*
> * All keys and values in opti->conflicted are a subset of those in
> * opti->paths. We don't want to deallocate anything twice, so we
> * don't free the keys and we pass 0 for free_values.
> */
> - strmap_func(&opti->conflicted, 0);
> + strmap_clear(&opti->conflicted, flags);
>
> if (!opti->pool) {
> /*
> @@ -579,24 +574,24 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
>
> /* Free memory used by various renames maps */
> for (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) {
> - strintmap_func(&renames->dirs_removed[i]);
> - strmap_func(&renames->dir_renames[i], 0);
> - strintmap_func(&renames->relevant_sources[i]);
> + strintmap_clear(&renames->dirs_removed[i], flags);
> + strmap_clear(&renames->dir_renames[i], flags);
> + strintmap_clear(&renames->relevant_sources[i], flags);
> if (!reinitialize)
> assert(renames->cached_pairs_valid_side == 0);
> if (i != renames->cached_pairs_valid_side &&
> -1 != renames->cached_pairs_valid_side) {
> - strset_func(&renames->cached_target_names[i]);
> - strmap_func(&renames->cached_pairs[i], 1);
> - strset_func(&renames->cached_irrelevant[i]);
> + strset_clear(&renames->cached_target_names[i], flags);
> + strmap_clear(&renames->cached_pairs[i], flags | STRMAP_FREE_VALUES);
> + strset_clear(&renames->cached_irrelevant[i], flags);
> partial_clear_dir_rename_count(&renames->dir_rename_count[i]);
> if (!reinitialize)
> strmap_clear(&renames->dir_rename_count[i], 1);
> }
> }
> for (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) {
> - strintmap_func(&renames->deferred[i].possible_trivial_merges);
> - strset_func(&renames->deferred[i].target_dirs);
> + strintmap_clear(&renames->deferred[i].possible_trivial_merges, flags);
> + strset_clear(&renames->deferred[i].target_dirs, flags);
> renames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */
> }
> renames->cached_pairs_valid_side = 0;
> @@ -1482,7 +1477,7 @@ static int handle_deferred_entries(struct merge_options *opt,
> if (ret < 0)
> return ret;
> }
> - strintmap_clear(©);
> + strintmap_clear(©, 0);
> strintmap_for_each_entry(&renames->deferred[side].possible_trivial_merges,
> &iter, entry) {
> const char *path = entry->key;
> diff --git a/strmap.c b/strmap.c
> index 4fb9f6100e..7343800df5 100644
> --- a/strmap.c
> +++ b/strmap.c
> @@ -37,10 +37,11 @@ void strmap_init_with_options(struct strmap *map,
> map->strdup_strings = strdup_strings;
> }
>
> -static void strmap_free_entries_(struct strmap *map, int free_values)
> +static void strmap_free_entries_(struct strmap *map, unsigned flags)
> {
> struct hashmap_iter iter;
> struct strmap_entry *e;
> + int free_values = flags & STRMAP_FREE_VALUES;
>
> if (!map)
> return;
> @@ -64,16 +65,13 @@ static void strmap_free_entries_(struct strmap *map, int free_values)
> }
> }
>
> -void strmap_clear(struct strmap *map, int free_values)
> +void strmap_clear(struct strmap *map, unsigned flags)
> {
> - strmap_free_entries_(map, free_values);
> - hashmap_clear(&map->map);
> -}
> -
> -void strmap_partial_clear(struct strmap *map, int free_values)
> -{
> - strmap_free_entries_(map, free_values);
> - hashmap_partial_clear(&map->map);
> + strmap_free_entries_(map, flags);
> + if (flags & STRMAP_PARTIAL_CLEAR)
> + hashmap_partial_clear(&map->map);
> + else
> + hashmap_clear(&map->map);
> }
>
> static struct strmap_entry *create_entry(struct strmap *map,
> diff --git a/strmap.h b/strmap.h
> index 1e152d832d..d03d451654 100644
> --- a/strmap.h
> +++ b/strmap.h
> @@ -46,16 +46,14 @@ void strmap_init_with_options(struct strmap *map,
> struct mem_pool *pool,
> int strdup_strings);
>
> -/*
> - * Remove all entries from the map, releasing any allocated resources.
> - */
> -void strmap_clear(struct strmap *map, int free_values);
> +#define STRMAP_FREE_VALUES 1 /* 1 for historical compat, but we should probably
> + update callers to use the correct name) */
> +#define STRMAP_PARTIAL_CLEAR 2
>
> /*
> - * Similar to strmap_clear() but leaves map->map->table allocated and
> - * pre-sized so that subsequent uses won't need as many rehashings.
> + * Remove all entries from the map, releasing any allocated resources.
> */
> -void strmap_partial_clear(struct strmap *map, int free_values);
> +void strmap_clear(struct strmap *map, unsigned flags);
>
> /*
> * Insert "str" into the map, pointing to "data".
> @@ -148,14 +146,10 @@ static inline void strintmap_init_with_options(struct strintmap *map,
> map->default_value = default_value;
> }
>
> -static inline void strintmap_clear(struct strintmap *map)
> -{
> - strmap_clear(&map->map, 0);
> -}
> -
> -static inline void strintmap_partial_clear(struct strintmap *map)
> +static inline void strintmap_clear(struct strintmap *map, unsigned flags)
> {
> - strmap_partial_clear(&map->map, 0);
> + /* maybe clear STRMAP_FREE_VALUES bit for extra protection */
> + strmap_clear(&map->map, flags);
> }
>
> static inline int strintmap_contains(struct strintmap *map, const char *str)
> @@ -232,14 +226,9 @@ static inline void strset_init_with_options(struct strset *set,
> strmap_init_with_options(&set->map, pool, strdup_strings);
> }
>
> -static inline void strset_clear(struct strset *set)
> -{
> - strmap_clear(&set->map, 0);
> -}
> -
> -static inline void strset_partial_clear(struct strset *set)
> +static inline void strset_clear(struct strset *set, unsigned flags)
> {
> - strmap_partial_clear(&set->map, 0);
> + strmap_clear(&set->map, flags);
> }
>
> static inline int strset_contains(struct strset *set, const char *str)
