| Message ID | 20190924010324.22619-19-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/19] diff: use hashmap_entry_init on moved_entry.ent | expand |
Eric Wong <e@80x24.org> writes: > While we cannot rely on a `__typeof__' operator being portable > to use with `offsetof'; we can calculate the pointer offset > using an existing pointer and the address of a member using > pointer arithmetic. > +/* > + * like offsetof(), but takes a pointer to type instead of the type It actually takes "a pointer to a variable of the type". I had to read the above twice to guess what is going on. > + * @ptr is subject to multiple evaluation since we can't rely on TYPEOF() > + */ > +#define OFFSETOF_VAR(ptr, member) \ > + ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) > + This unfortunately has funny interactions with using uninitialized variables. "make CC=clang config.o" gives something like this: config.c:1944:50: error: variable 'entry' is uninitialized when used here [-Werror,-Wuninitialized] hashmap_for_each_entry(&cs->config_hash, &iter, entry, ^~~~~ ./hashmap.h:453:20: note: expanded from macro 'hashmap_for_each_entry' OFFSETOF_VAR(var, member)); \ ^~~ ./git-compat-util.h:1346:16: note: expanded from macro 'OFFSETOF_VAR' ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) ^~~ ./hashmap.h:445:61: note: expanded from macro 'hashmap_iter_first_entry_offset' container_of_or_null_offset(hashmap_iter_first(map, iter), offset) ^~~~~~ config.c:1939:34: note: initialize the variable 'entry' to silence this warning struct config_set_element *entry; ^ = NULL Personally, I feel the workaround suggested by the compiler is just as bogus as the warning itself X-<. Casting NULL as if it were a pointer to an object of type typeof(*ptr), treating it as if it is a valid address, and doing raw address arith is just as bogus as doing the same raw address arith on an uninitialized ptr, isn't it? Anyway...
Eric Wong <e@80x24.org> writes: > In the future, list iterator macros (e.g. list_for_each_entry) > may also be implemented using OFFSETOF_VAR to save hackers the > trouble of using container_of/list_entry macros and without > relying on non-portable `__typeof__'. Can we add something like this as a preliminary preparation step before the series? Subject: [PATCH] treewide: initialize pointers to hashmap entries There are not strictly necessary, but some compilers (e.g. clang 6.0.1) apparently have trouble in construct we will use in the OFFSETOF_VAR() macro, i.e. ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) when the ptr is uninitialized. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- attr.c | 2 +- blame.c | 4 ++-- builtin/describe.c | 2 +- builtin/difftool.c | 2 +- config.c | 2 +- merge-recursive.c | 6 +++--- revision.c | 4 ++-- submodule-config.c | 2 +- t/helper/test-hashmap.c | 2 +- t/helper/test-lazy-init-name-hash.c | 4 ++-- 10 files changed, 15 insertions(+), 15 deletions(-) diff --git a/attr.c b/attr.c index 15f0efdf60..0c367fb84a 100644 --- a/attr.c +++ b/attr.c @@ -161,7 +161,7 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check) * field and fill each entry with its corresponding git_attr. */ if (size != check->all_attrs_nr) { - struct attr_hash_entry *e; + struct attr_hash_entry *e = NULL; struct hashmap_iter iter; REALLOC_ARRAY(check->all_attrs, size); diff --git a/blame.c b/blame.c index 6384f48133..a5f8672419 100644 --- a/blame.c +++ b/blame.c @@ -448,7 +448,7 @@ static int fingerprint_similarity(struct fingerprint *a, struct fingerprint *b) { int intersection = 0; struct hashmap_iter iter; - const struct fingerprint_entry *entry_a, *entry_b; + const struct fingerprint_entry *entry_a, *entry_b = NULL; hashmap_for_each_entry(&b->map, &iter, entry_b, entry /* member name */) { @@ -467,7 +467,7 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b) { struct hashmap_iter iter; struct fingerprint_entry *entry_a; - const struct fingerprint_entry *entry_b; + const struct fingerprint_entry *entry_b = NULL; hashmap_iter_init(&b->map, &iter); diff --git a/builtin/describe.c b/builtin/describe.c index 1caf98f716..42e6cc3a4f 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -330,7 +330,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) if (!have_util) { struct hashmap_iter iter; struct commit *c; - struct commit_name *n; + struct commit_name *n = NULL; init_commit_names(&commit_names); hashmap_for_each_entry(&names, &iter, n, diff --git a/builtin/difftool.c b/builtin/difftool.c index c280e682b2..30f3bd6219 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -344,7 +344,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, FILE *fp; struct hashmap working_tree_dups, submodules, symlinks2; struct hashmap_iter iter; - struct pair_entry *entry; + struct pair_entry *entry = NULL; struct index_state wtindex; struct checkout lstate, rstate; int rc, flags = RUN_GIT_CMD, err = 0; diff --git a/config.c b/config.c index a4fa464ed2..6ceefb7cff 100644 --- a/config.c +++ b/config.c @@ -1936,7 +1936,7 @@ void git_configset_init(struct config_set *cs) void git_configset_clear(struct config_set *cs) { - struct config_set_element *entry; + struct config_set_element *entry = NULL; struct hashmap_iter iter; if (!cs->hash_initialized) return; diff --git a/merge-recursive.c b/merge-recursive.c index 8787a40b0c..bc826fb7ac 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2127,7 +2127,7 @@ static void handle_directory_level_conflicts(struct merge_options *opt, struct tree *merge) { struct hashmap_iter iter; - struct dir_rename_entry *head_ent; + struct dir_rename_entry *head_ent = NULL; struct dir_rename_entry *merge_ent; struct string_list remove_from_head = STRING_LIST_INIT_NODUP; @@ -2566,7 +2566,7 @@ static struct string_list *get_renames(struct merge_options *opt, int i; struct hashmap collisions; struct hashmap_iter iter; - struct collision_entry *e; + struct collision_entry *e = NULL; struct string_list *renames; compute_collisions(&collisions, dir_renames, pairs); @@ -2838,7 +2838,7 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs, struct hashmap *dir_renames) { struct hashmap_iter iter; - struct dir_rename_entry *e; + struct dir_rename_entry *e = NULL; hashmap_for_each_entry(dir_renames, &iter, e, ent /* member name */) { diff --git a/revision.c b/revision.c index 6688f89d0d..70f478666b 100644 --- a/revision.c +++ b/revision.c @@ -127,7 +127,7 @@ static void paths_and_oids_init(struct hashmap *map) static void paths_and_oids_clear(struct hashmap *map) { struct hashmap_iter iter; - struct path_and_oids_entry *entry; + struct path_and_oids_entry *entry = NULL; hashmap_for_each_entry(map, &iter, entry, ent /* member name */) { oidset_clear(&entry->trees); @@ -210,7 +210,7 @@ void mark_trees_uninteresting_sparse(struct repository *r, unsigned has_interesting = 0, has_uninteresting = 0; struct hashmap map; struct hashmap_iter map_iter; - struct path_and_oids_entry *entry; + struct path_and_oids_entry *entry = NULL; struct object_id *oid; struct oidset_iter iter; diff --git a/submodule-config.c b/submodule-config.c index 401a9b2382..4d4d3b7dd7 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -89,7 +89,7 @@ static void free_one_config(struct submodule_entry *entry) static void submodule_cache_clear(struct submodule_cache *cache) { struct hashmap_iter iter; - struct submodule_entry *entry; + struct submodule_entry *entry = NULL; if (!cache->initialized) return; diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c index cc577c8956..a3bed8cc70 100644 --- a/t/helper/test-hashmap.c +++ b/t/helper/test-hashmap.c @@ -161,7 +161,7 @@ int cmd__hashmap(int argc, const char **argv) while (strbuf_getline(&line, stdin) != EOF) { char *cmd, *p1 = NULL, *p2 = NULL; unsigned int hash = 0; - struct test_entry *entry; + struct test_entry *entry = NULL; /* break line into command and up to two parameters */ cmd = strtok(line.buf, DELIM); diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c index cd1b4c9736..2cb1fa3d8c 100644 --- a/t/helper/test-lazy-init-name-hash.c +++ b/t/helper/test-lazy-init-name-hash.c @@ -29,8 +29,8 @@ static void dump_run(void) char name[FLEX_ARRAY]; }; - struct dir_entry *dir; - struct cache_entry *ce; + struct dir_entry *dir = NULL; + struct cache_entry *ce = NULL; read_cache(); if (single) {
Junio C Hamano <gitster@pobox.com> wrote: > Eric Wong <e@80x24.org> writes: > > > In the future, list iterator macros (e.g. list_for_each_entry) > > may also be implemented using OFFSETOF_VAR to save hackers the > > trouble of using container_of/list_entry macros and without > > relying on non-portable `__typeof__'. > > Can we add something like this as a preliminary preparation step > before the series? > > Subject: [PATCH] treewide: initialize pointers to hashmap entries > > There are not strictly necessary, but some compilers (e.g. clang > 6.0.1) apparently have trouble in construct we will use in the > OFFSETOF_VAR() macro, i.e. > > ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) > > when the ptr is uninitialized. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > attr.c | 2 +- > blame.c | 4 ++-- > builtin/describe.c | 2 +- > builtin/difftool.c | 2 +- > config.c | 2 +- > merge-recursive.c | 6 +++--- > revision.c | 4 ++-- > submodule-config.c | 2 +- > t/helper/test-hashmap.c | 2 +- > t/helper/test-lazy-init-name-hash.c | 4 ++-- > 10 files changed, 15 insertions(+), 15 deletions(-) That seems too tedious. I'm learning towards just initializing var = NULL in the start of the for-loop: @@ -449,7 +449,8 @@ static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map, * containing a @member which is a "struct hashmap_entry" */ #define hashmap_for_each_entry(map, iter, var, member) \ - for (var = hashmap_iter_first_entry_offset(map, iter, \ + for (var = NULL /* squelch uninitialized warnings for OFFSETOF_VAR */, \ + var = hashmap_iter_first_entry_offset(map, iter, \ OFFSETOF_VAR(var, member)); \ var; \ var = hashmap_iter_next_entry_offset(iter, \ (But I'm running on fumes all week, so not sure I trust it)
Eric Wong <e@80x24.org> writes: > That seems too tedious. I'm learning towards just initializing > var = NULL in the start of the for-loop: > > @@ -449,7 +449,8 @@ static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map, > * containing a @member which is a "struct hashmap_entry" > */ > #define hashmap_for_each_entry(map, iter, var, member) \ > - for (var = hashmap_iter_first_entry_offset(map, iter, \ > + for (var = NULL /* squelch uninitialized warnings for OFFSETOF_VAR */, \ > + var = hashmap_iter_first_entry_offset(map, iter, \ > OFFSETOF_VAR(var, member)); \ That looks a bit too ugly for my taste. I've added an updated version (see below) and then rebased the whole thing on top of it. "hashmap: use *_entry APIs for iteration" needs a trivial textual conflict resolved, but otherwise the result looked OK. -- >8 -- From: Junio C Hamano <gitster@pobox.com> Date: Fri, 4 Oct 2019 10:12:25 +0900 Subject: [PATCH] treewide: initialize pointers to hashmap entries There are not strictly necessary to avoid use of uninitialized variables, but some compilers (e.g. clang 6.0.1) apparently have trouble in construct we will use in the OFFSETOF_VAR() macro, i.e. ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) when the ptr is uninitialized, treating such a "ptr" as "used". It is just as bogus to subtract NULL from the address of the "member" field pretending the structure sits at NULL address as doing so with a structure that sits at a random address, but apparently clang issues a warning with an advice to initialize the pointer to NULL, so let's do that. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- attr.c | 2 +- blame.c | 4 ++-- builtin/describe.c | 2 +- builtin/difftool.c | 2 +- config.c | 2 +- merge-recursive.c | 6 +++--- revision.c | 4 ++-- submodule-config.c | 2 +- t/helper/test-hashmap.c | 2 +- t/helper/test-lazy-init-name-hash.c | 4 ++-- 10 files changed, 15 insertions(+), 15 deletions(-) diff --git a/attr.c b/attr.c index 93dc16b59c..cb85303a06 100644 --- a/attr.c +++ b/attr.c @@ -159,7 +159,7 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check) * field and fill each entry with its corresponding git_attr. */ if (size != check->all_attrs_nr) { - struct attr_hash_entry *e; + struct attr_hash_entry *e = NULL; struct hashmap_iter iter; hashmap_iter_init(&map->map, &iter); diff --git a/blame.c b/blame.c index 36a2e7ef11..9b912867f7 100644 --- a/blame.c +++ b/blame.c @@ -447,7 +447,7 @@ static int fingerprint_similarity(struct fingerprint *a, struct fingerprint *b) { int intersection = 0; struct hashmap_iter iter; - const struct fingerprint_entry *entry_a, *entry_b; + const struct fingerprint_entry *entry_a, *entry_b = NULL; hashmap_iter_init(&b->map, &iter); @@ -466,7 +466,7 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b) { struct hashmap_iter iter; struct fingerprint_entry *entry_a; - const struct fingerprint_entry *entry_b; + const struct fingerprint_entry *entry_b = NULL; hashmap_iter_init(&b->map, &iter); diff --git a/builtin/describe.c b/builtin/describe.c index 200154297d..0369ed66ce 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -327,7 +327,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) if (!have_util) { struct hashmap_iter iter; struct commit *c; - struct commit_name *n; + struct commit_name *n = NULL; init_commit_names(&commit_names); n = hashmap_iter_first(&names, &iter); diff --git a/builtin/difftool.c b/builtin/difftool.c index 16eb8b70ea..fbb4c87a86 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -337,7 +337,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, FILE *fp; struct hashmap working_tree_dups, submodules, symlinks2; struct hashmap_iter iter; - struct pair_entry *entry; + struct pair_entry *entry = NULL; struct index_state wtindex; struct checkout lstate, rstate; int rc, flags = RUN_GIT_CMD, err = 0; diff --git a/config.c b/config.c index 3900e4947b..6c92d2825d 100644 --- a/config.c +++ b/config.c @@ -1934,7 +1934,7 @@ void git_configset_init(struct config_set *cs) void git_configset_clear(struct config_set *cs) { - struct config_set_element *entry; + struct config_set_element *entry = NULL; struct hashmap_iter iter; if (!cs->hash_initialized) return; diff --git a/merge-recursive.c b/merge-recursive.c index 6b812d67e3..ea17d9c30f 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2118,7 +2118,7 @@ static void handle_directory_level_conflicts(struct merge_options *opt, struct tree *merge) { struct hashmap_iter iter; - struct dir_rename_entry *head_ent; + struct dir_rename_entry *head_ent = NULL; struct dir_rename_entry *merge_ent; struct string_list remove_from_head = STRING_LIST_INIT_NODUP; @@ -2556,7 +2556,7 @@ static struct string_list *get_renames(struct merge_options *opt, int i; struct hashmap collisions; struct hashmap_iter iter; - struct collision_entry *e; + struct collision_entry *e = NULL; struct string_list *renames; compute_collisions(&collisions, dir_renames, pairs); @@ -2828,7 +2828,7 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs, struct hashmap *dir_renames) { struct hashmap_iter iter; - struct dir_rename_entry *e; + struct dir_rename_entry *e = NULL; hashmap_iter_init(dir_renames, &iter); while ((e = hashmap_iter_next(&iter))) { diff --git a/revision.c b/revision.c index 07412297f0..b9b538c4d2 100644 --- a/revision.c +++ b/revision.c @@ -122,7 +122,7 @@ static void paths_and_oids_init(struct hashmap *map) static void paths_and_oids_clear(struct hashmap *map) { struct hashmap_iter iter; - struct path_and_oids_entry *entry; + struct path_and_oids_entry *entry = NULL; hashmap_iter_init(map, &iter); while ((entry = (struct path_and_oids_entry *)hashmap_iter_next(&iter))) { @@ -205,7 +205,7 @@ void mark_trees_uninteresting_sparse(struct repository *r, unsigned has_interesting = 0, has_uninteresting = 0; struct hashmap map; struct hashmap_iter map_iter; - struct path_and_oids_entry *entry; + struct path_and_oids_entry *entry = NULL; struct object_id *oid; struct oidset_iter iter; diff --git a/submodule-config.c b/submodule-config.c index 4264ee216f..af86fe9ac1 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -85,7 +85,7 @@ static void free_one_config(struct submodule_entry *entry) static void submodule_cache_clear(struct submodule_cache *cache) { struct hashmap_iter iter; - struct submodule_entry *entry; + struct submodule_entry *entry = NULL; if (!cache->initialized) return; diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c index aaf17b0ddf..6407e0b426 100644 --- a/t/helper/test-hashmap.c +++ b/t/helper/test-hashmap.c @@ -159,7 +159,7 @@ int cmd__hashmap(int argc, const char **argv) while (strbuf_getline(&line, stdin) != EOF) { char *cmd, *p1 = NULL, *p2 = NULL; unsigned int hash = 0; - struct test_entry *entry; + struct test_entry *entry = NULL; /* break line into command and up to two parameters */ cmd = strtok(line.buf, DELIM); diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c index b99a37080d..95ab81ee7a 100644 --- a/t/helper/test-lazy-init-name-hash.c +++ b/t/helper/test-lazy-init-name-hash.c @@ -29,8 +29,8 @@ static void dump_run(void) char name[FLEX_ARRAY]; }; - struct dir_entry *dir; - struct cache_entry *ce; + struct dir_entry *dir = NULL; + struct cache_entry *ce = NULL; read_cache(); if (single) {
Junio C Hamano <gitster@pobox.com> wrote: > Eric Wong <e@80x24.org> writes: > > > That seems too tedious. I'm learning towards just initializing > > var = NULL in the start of the for-loop: > > > > @@ -449,7 +449,8 @@ static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map, > > * containing a @member which is a "struct hashmap_entry" > > */ > > #define hashmap_for_each_entry(map, iter, var, member) \ > > - for (var = hashmap_iter_first_entry_offset(map, iter, \ > > + for (var = NULL /* squelch uninitialized warnings for OFFSETOF_VAR */, \ > > + var = hashmap_iter_first_entry_offset(map, iter, \ > > OFFSETOF_VAR(var, member)); \ > > That looks a bit too ugly for my taste. I've added an updated > version (see below) and then rebased the whole thing on top of it. I prefer to minimize the amount of stuff we do to work around buggy compilers (in case they get fixed and old versions are obsoleted). If it's just clang with this problem, we know clang sets __GNUC__, so we can use __typeof__ directly (bypassing extra parentheses in our TYPEOF macro) to get around the warning: #if defined(__GNUC__) /* clang sets this, too */ #define OFFSETOF_VAR(ptr, member) offsetof(__typeof__(*ptr), member) #else /* !__GNUC__ */ ... That said, there could be other compilers which don't set __GNUC__ and have the same problem as clang. But maybe those compilers are too buggy and we already ignore invalid warnings on those compilers.
Eric Wong <e@80x24.org> writes: > That said, there could be other compilers which don't set > __GNUC__ and have the same problem as clang. But maybe those > compilers are too buggy and we already ignore invalid warnings > on those compilers. Perhaps.
diff --git a/attr.c b/attr.c index ca8be46e8e..9849106627 100644 --- a/attr.c +++ b/attr.c @@ -168,7 +168,6 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check) check->all_attrs_nr = size; hashmap_for_each_entry(&map->map, &iter, e, - struct attr_hash_entry, ent /* member name */) { const struct git_attr *a = e->value; check->all_attrs[a->attr_nr].attr = a; diff --git a/blame.c b/blame.c index f33af0da9f..90b247abf9 100644 --- a/blame.c +++ b/blame.c @@ -451,7 +451,6 @@ static int fingerprint_similarity(struct fingerprint *a, struct fingerprint *b) const struct fingerprint_entry *entry_a, *entry_b; hashmap_for_each_entry(&b->map, &iter, entry_b, - const struct fingerprint_entry, entry /* member name */) { entry_a = hashmap_get_entry(&a->map, entry_b, NULL, struct fingerprint_entry, entry); @@ -474,7 +473,6 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b) hashmap_iter_init(&b->map, &iter); hashmap_for_each_entry(&b->map, &iter, entry_b, - const struct fingerprint_entry, entry /* member name */) { entry_a = hashmap_get_entry(&a->map, entry_b, NULL, struct fingerprint_entry, entry); diff --git a/builtin/describe.c b/builtin/describe.c index 8cf2cd992d..1caf98f716 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -333,7 +333,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) struct commit_name *n; init_commit_names(&commit_names); - hashmap_for_each_entry(&names, &iter, n, struct commit_name, + hashmap_for_each_entry(&names, &iter, n, entry /* member name */) { c = lookup_commit_reference_gently(the_repository, &n->peeled, 1); diff --git a/builtin/difftool.c b/builtin/difftool.c index dd94179b68..f2d4d1e0f8 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -539,7 +539,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, * change in the recorded SHA1 for the submodule. */ hashmap_for_each_entry(&submodules, &iter, entry, - struct pair_entry, entry /* member name */) { + entry /* member name */) { if (*entry->left) { add_path(&ldir, ldir_len, entry->path); ensure_leading_directories(ldir.buf); @@ -558,7 +558,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, * This loop replicates that behavior. */ hashmap_for_each_entry(&symlinks2, &iter, entry, - struct pair_entry, entry /* member name */) { + entry /* member name */) { if (*entry->left) { add_path(&ldir, ldir_len, entry->path); ensure_leading_directories(ldir.buf); diff --git a/config.c b/config.c index 4d05dbc15a..77ed00bfbf 100644 --- a/config.c +++ b/config.c @@ -1943,7 +1943,6 @@ void git_configset_clear(struct config_set *cs) return; hashmap_for_each_entry(&cs->config_hash, &iter, entry, - struct config_set_element, ent /* member name */) { free(entry->key); string_list_clear(&entry->value_list, 1); diff --git a/diff.c b/diff.c index f94d9f96af..051de9832d 100644 --- a/diff.c +++ b/diff.c @@ -1038,7 +1038,7 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o, int i; char *got_match = xcalloc(1, pmb_nr); - hashmap_for_each_entry_from(hm, match, struct moved_entry, ent) { + hashmap_for_each_entry_from(hm, match, ent) { for (i = 0; i < pmb_nr; i++) { struct moved_entry *prev = pmb[i].match; struct moved_entry *cur = (prev && prev->next_line) ? @@ -1193,8 +1193,7 @@ static void mark_color_as_moved(struct diff_options *o, * The current line is the start of a new block. * Setup the set of potential blocks. */ - hashmap_for_each_entry_from(hm, match, - struct moved_entry, ent) { + hashmap_for_each_entry_from(hm, match, ent) { ALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc); if (o->color_moved_ws_handling & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) { diff --git a/diffcore-rename.c b/diffcore-rename.c index 994609ed58..9ad4dc395a 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -284,7 +284,7 @@ static int find_identical_files(struct hashmap *srcs, */ p = hashmap_get_entry_from_hash(srcs, hash, NULL, struct file_similarity, entry); - hashmap_for_each_entry_from(srcs, p, struct file_similarity, entry) { + hashmap_for_each_entry_from(srcs, p, entry) { int score; struct diff_filespec *source = p->filespec; diff --git a/git-compat-util.h b/git-compat-util.h index e24510452a..b3dbb5a3c9 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1338,4 +1338,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) #define container_of_or_null(ptr, type, member) \ (type *)container_of_or_null_offset(ptr, offsetof(type, member)) +/* + * like offsetof(), but takes a pointer to type instead of the type + * @ptr is subject to multiple evaluation since we can't rely on TYPEOF() + */ +#define OFFSETOF_VAR(ptr, member) \ + ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) + #endif diff --git a/hashmap.h b/hashmap.h index 7c7a54d15e..519213a812 100644 --- a/hashmap.h +++ b/hashmap.h @@ -408,16 +408,32 @@ static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map, return hashmap_iter_next(iter); } -#define hashmap_iter_next_entry(iter, type, member) \ - container_of_or_null(hashmap_iter_next(iter), type, member) - +/* + * returns the first entry in @map using @iter, where the entry is of + * @type (e.g. "struct foo") and @member is the name of the + * "struct hashmap_entry" in @type + */ #define hashmap_iter_first_entry(map, iter, type, member) \ container_of_or_null(hashmap_iter_first(map, iter), type, member) -#define hashmap_for_each_entry(map, iter, var, type, member) \ - for (var = hashmap_iter_first_entry(map, iter, type, member); \ +/* internal macro for hashmap_for_each_entry */ +#define hashmap_iter_next_entry_offset(iter, offset) \ + container_of_or_null_offset(hashmap_iter_next(iter), offset) + +/* internal macro for hashmap_for_each_entry */ +#define hashmap_iter_first_entry_offset(map, iter, offset) \ + container_of_or_null_offset(hashmap_iter_first(map, iter), offset) + +/* + * iterate through @map using @iter, @var is a pointer to a type + * containing a @member which is a "struct hashmap_entry" + */ +#define hashmap_for_each_entry(map, iter, var, member) \ + for (var = hashmap_iter_first_entry_offset(map, iter, \ + OFFSETOF_VAR(var, member)); \ var; \ - var = hashmap_iter_next_entry(iter, type, member)) + var = hashmap_iter_next_entry_offset(iter, \ + OFFSETOF_VAR(var, member))) /* * returns a @pointer of @type matching @keyvar, or NULL if nothing found. @@ -432,22 +448,22 @@ static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map, container_of_or_null(hashmap_get_from_hash(map, hash, keydata), \ type, member) /* - * returns the next equal @type pointer to @var, or NULL if not found. - * @var is a pointer of @type - * @member is the name of the "struct hashmap_entry" field in @type + * returns the next equal pointer to @var, or NULL if not found. + * @var is a pointer of any type containing "struct hashmap_entry" + * @member is the name of the "struct hashmap_entry" field */ -#define hashmap_get_next_entry(map, var, type, member) \ - container_of_or_null(hashmap_get_next(map, &(var)->member), \ - type, member) +#define hashmap_get_next_entry(map, var, member) \ + container_of_or_null_offset(hashmap_get_next(map, &(var)->member), \ + OFFSETOF_VAR(var, member)) /* * iterate @map starting from @var, where @var is a pointer of @type * and @member is the name of the "struct hashmap_entry" field in @type */ -#define hashmap_for_each_entry_from(map, var, type, member) \ +#define hashmap_for_each_entry_from(map, var, member) \ for (; \ var; \ - var = hashmap_get_next_entry(map, var, type, member)) + var = hashmap_get_next_entry(map, var, member)) /* * Disable item counting and automatic rehashing when adding/removing items. diff --git a/merge-recursive.c b/merge-recursive.c index 34b3d54154..3abba3a618 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2136,7 +2136,6 @@ static void handle_directory_level_conflicts(struct merge_options *opt, struct string_list remove_from_merge = STRING_LIST_INIT_NODUP; hashmap_for_each_entry(dir_re_head, &iter, head_ent, - struct dir_rename_entry, ent /* member name */) { merge_ent = dir_rename_find_entry(dir_re_merge, head_ent->dir); if (merge_ent && @@ -2162,7 +2161,6 @@ static void handle_directory_level_conflicts(struct merge_options *opt, remove_hashmap_entries(dir_re_merge, &remove_from_merge); hashmap_for_each_entry(dir_re_merge, &iter, merge_ent, - struct dir_rename_entry, ent /* member name */) { head_ent = dir_rename_find_entry(dir_re_head, merge_ent->dir); if (tree_has_path(opt->repo, merge, merge_ent->dir)) { @@ -2268,7 +2266,6 @@ static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs) * that there is no winner), we no longer need possible_new_dirs. */ hashmap_for_each_entry(dir_renames, &iter, entry, - struct dir_rename_entry, ent /* member name */) { int max = 0; int bad_max = 0; @@ -2628,7 +2625,6 @@ static struct string_list *get_renames(struct merge_options *opt, } hashmap_for_each_entry(&collisions, &iter, e, - struct collision_entry, ent /* member name */) { free(e->target_file); string_list_clear(&e->source_files, 0); @@ -2847,7 +2843,6 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs, struct dir_rename_entry *e; hashmap_for_each_entry(dir_renames, &iter, e, - struct dir_rename_entry, ent /* member name */) { free(e->dir); strbuf_release(&e->new_dir); diff --git a/name-hash.c b/name-hash.c index c86fe0f1df..3cda22b657 100644 --- a/name-hash.c +++ b/name-hash.c @@ -714,8 +714,7 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na ce = hashmap_get_entry_from_hash(&istate->name_hash, hash, NULL, struct cache_entry, ent); - hashmap_for_each_entry_from(&istate->name_hash, ce, - struct cache_entry, ent) { + hashmap_for_each_entry_from(&istate->name_hash, ce, ent) { if (same_name(ce, name, namelen, icase)) return ce; } diff --git a/revision.c b/revision.c index 8a5f866ae6..5abd4a1fe7 100644 --- a/revision.c +++ b/revision.c @@ -129,9 +129,7 @@ static void paths_and_oids_clear(struct hashmap *map) struct hashmap_iter iter; struct path_and_oids_entry *entry; - hashmap_for_each_entry(map, &iter, entry, - struct path_and_oids_entry, - ent /* member name */) { + hashmap_for_each_entry(map, &iter, entry, ent /* member name */) { oidset_clear(&entry->trees); free(entry->path); } @@ -243,9 +241,7 @@ void mark_trees_uninteresting_sparse(struct repository *r, add_children_by_path(r, tree, &map); } - hashmap_for_each_entry(&map, &map_iter, entry, - struct path_and_oids_entry, - ent /* member name */) + hashmap_for_each_entry(&map, &map_iter, entry, ent /* member name */) mark_trees_uninteresting_sparse(r, &entry->trees); paths_and_oids_clear(&map); diff --git a/submodule-config.c b/submodule-config.c index 5462acc8ec..c22855cd38 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -100,7 +100,7 @@ static void submodule_cache_clear(struct submodule_cache *cache) * their .gitmodules blob sha1 and submodule name. */ hashmap_for_each_entry(&cache->for_name, &iter, entry, - struct submodule_entry, ent /* member name */) + ent /* member name */) free_one_config(entry); hashmap_free_entries(&cache->for_path, struct submodule_entry, ent); diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c index 6f2530dcc8..f89d1194ef 100644 --- a/t/helper/test-hashmap.c +++ b/t/helper/test-hashmap.c @@ -205,10 +205,8 @@ int cmd__hashmap(int argc, const char **argv) /* print result */ if (!entry) puts("NULL"); - hashmap_for_each_entry_from(&map, entry, - struct test_entry, ent) { + hashmap_for_each_entry_from(&map, entry, ent) puts(get_value(entry)); - } } else if (!strcmp("remove", cmd) && p1) { @@ -230,7 +228,6 @@ int cmd__hashmap(int argc, const char **argv) struct hashmap_iter iter; hashmap_for_each_entry(&map, &iter, entry, - struct test_entry, ent /* member name */) printf("%s %s\n", entry->key, get_value(entry)); diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c index 9d4664d6a4..cd1b4c9736 100644 --- a/t/helper/test-lazy-init-name-hash.c +++ b/t/helper/test-lazy-init-name-hash.c @@ -42,11 +42,11 @@ static void dump_run(void) } hashmap_for_each_entry(&the_index.dir_hash, &iter_dir, dir, - struct dir_entry, ent /* member name */) + ent /* member name */) printf("dir %08x %7d %s\n", dir->ent.hash, dir->nr, dir->name); hashmap_for_each_entry(&the_index.name_hash, &iter_cache, ce, - struct cache_entry, ent /* member name */) + ent /* member name */) printf("name %08x %s\n", ce->ent.hash, ce->name); discard_cache();
While we cannot rely on a `__typeof__' operator being portable to use with `offsetof'; we can calculate the pointer offset using an existing pointer and the address of a member using pointer arithmetic. This allows us to simplify usage of hashmap iterator macros by not having to specify a type when a pointer of that type is already given. In the future, list iterator macros (e.g. list_for_each_entry) may also be implemented using OFFSETOF_VAR to save hackers the trouble of using container_of/list_entry macros and without relying on non-portable `__typeof__'. Signed-off-by: Eric Wong <e@80x24.org> --- attr.c | 1 - blame.c | 2 -- builtin/describe.c | 2 +- builtin/difftool.c | 4 +-- config.c | 1 - diff.c | 5 ++-- diffcore-rename.c | 2 +- git-compat-util.h | 7 +++++ hashmap.h | 44 ++++++++++++++++++++--------- merge-recursive.c | 5 ---- name-hash.c | 3 +- revision.c | 8 ++---- submodule-config.c | 2 +- t/helper/test-hashmap.c | 5 +--- t/helper/test-lazy-init-name-hash.c | 4 +-- 15 files changed, 50 insertions(+), 45 deletions(-)