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).
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)
