| Message ID | 20190826024332.3403-12-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] diff: use hashmap_entry_init on moved_entry.ent | expand |
On 8/25/2019 10:43 PM, Eric Wong wrote: > This is a step towards removing the requirement for > hashmap_entry being the first field of a struct. > > Signed-off-by: Eric Wong <e@80x24.org> > --- > diff.c | 19 ++++++++++++------- > diffcore-rename.c | 11 +++++++---- > hashmap.c | 2 +- > hashmap.h | 12 ++++++++---- > name-hash.c | 8 +++++--- > t/helper/test-hashmap.c | 10 ++++++---- > 6 files changed, 39 insertions(+), 23 deletions(-) > > diff --git a/diff.c b/diff.c > index 72d3c6aa19..663b5d01f8 100644 > --- a/diff.c > +++ b/diff.c > @@ -1035,8 +1035,10 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o, > { > int i; > char *got_match = xcalloc(1, pmb_nr); > + struct hashmap_entry *ent = &match->ent; > > - for (; match; match = hashmap_get_next(hm, &match->ent)) { > + for (; ent; ent = hashmap_get_next(hm, ent)) { > + match = container_of(ent, struct moved_entry, ent); Lines like this are very difficult to parse. In this container_of() macro, 'ent' is taking both the 'ptr' and 'member' values. I would prefer that you make your local member be named something different, for instance: struct hashmap_entry *match_ent = &match->ent; and match = container_of(match_ent, struct moved_entry, ent); > for (i = 0; i < pmb_nr; i++) { > struct moved_entry *prev = pmb[i].match; > struct moved_entry *cur = (prev && prev->next_line) ? > @@ -1135,8 +1137,9 @@ static void mark_color_as_moved(struct diff_options *o, > > for (n = 0; n < o->emitted_symbols->nr; n++) { > struct hashmap *hm = NULL; > + struct hashmap_entry *ent = NULL; > struct moved_entry *key; > - struct moved_entry *match = NULL; > + struct moved_entry *match; > struct emitted_diff_symbol *l = &o->emitted_symbols->buf[n]; > enum diff_symbol last_symbol = 0; > > @@ -1144,20 +1147,20 @@ static void mark_color_as_moved(struct diff_options *o, > case DIFF_SYMBOL_PLUS: > hm = del_lines; > key = prepare_entry(o, n); > - match = hashmap_get(hm, &key->ent, NULL); > + ent = hashmap_get(hm, &key->ent, NULL); > free(key); > break; > case DIFF_SYMBOL_MINUS: > hm = add_lines; > key = prepare_entry(o, n); > - match = hashmap_get(hm, &key->ent, NULL); > + ent = hashmap_get(hm, &key->ent, NULL); > free(key); > break; > default: > flipped_block = 0; > } > > - if (!match) { > + if (!ent) { > int i; > > adjust_last_block(o, n, block_length); > @@ -1169,6 +1172,7 @@ static void mark_color_as_moved(struct diff_options *o, > last_symbol = l->s; > continue; > } > + match = container_of(ent, struct moved_entry, ent); > > if (o->color_moved == COLOR_MOVED_PLAIN) { > last_symbol = l->s; > @@ -1189,8 +1193,9 @@ 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. > */ > - for (; match; match = hashmap_get_next(hm, > - &match->ent)) { > + for (; ent; ent = hashmap_get_next(hm, ent)) { > + match = container_of(ent, struct moved_entry, > + ent); Same complaint here. > 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 4670a40179..71aa240a68 100644 > --- a/diffcore-rename.c > +++ b/diffcore-rename.c > @@ -274,7 +274,7 @@ static int find_identical_files(struct hashmap *srcs, > struct diff_options *options) > { > int renames = 0; > - > + struct hashmap_entry *ent; > struct diff_filespec *target = rename_dst[dst_index].two; > struct file_similarity *p, *best = NULL; > int i = 100, best_score = -1; > @@ -282,12 +282,15 @@ static int find_identical_files(struct hashmap *srcs, > /* > * Find the best source match for specified destination. > */ > - p = hashmap_get_from_hash(srcs, > + ent = hashmap_get_from_hash(srcs, > hash_filespec(options->repo, target), > NULL); > - for (; p; p = hashmap_get_next(srcs, &p->entry)) { > + for (; ent; ent = hashmap_get_next(srcs, ent)) { > int score; > - struct diff_filespec *source = p->filespec; > + struct diff_filespec *source; > + > + p = container_of(ent, struct file_similarity, entry); This is slightly better, but still a bit confusing. > + source = p->filespec; > > /* False hash collision? */ > if (!oideq(&source->oid, &target->oid)) > diff --git a/hashmap.c b/hashmap.c > index 2dd9912e13..d6434d9ca4 100644 > --- a/hashmap.c > +++ b/hashmap.c > @@ -192,7 +192,7 @@ void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key, > return *find_entry_ptr(map, key, keydata); > } > > -void *hashmap_get_next(const struct hashmap *map, > +struct hashmap_entry *hashmap_get_next(const struct hashmap *map, > const struct hashmap_entry *entry) > { > struct hashmap_entry *e = entry->next; > diff --git a/hashmap.h b/hashmap.h > index b62ee2e7b9..25643dcdc4 100644 > --- a/hashmap.h > +++ b/hashmap.h > @@ -55,15 +55,19 @@ > * > * if (!strcmp("print_all_by_key", action)) { > * struct long2string k, *e; > + * struct hashmap_entry *ent; > * hashmap_entry_init(&k->ent, memhash(&key, sizeof(long))); > * k.key = key; > * > * flags &= ~COMPARE_VALUE; > - * e = hashmap_get(&map, &k, NULL); > - * if (e) { > + * ent = hashmap_get(&map, &k, NULL); > + * if (ent) { > + * e = container_of(ent, struct long2string, ent); > * printf("first: %ld %s\n", e->key, e->value); > - * while ((e = hashmap_get_next(&map, e))) > + * while ((ent = hashmap_get_next(&map, ent))) { > + * e = container_of(ent, struct long2string, ent); > * printf("found more: %ld %s\n", e->key, e->value); > + * } > * } > * } > * > @@ -320,7 +324,7 @@ static inline void *hashmap_get_from_hash(const struct hashmap *map, > * `entry` is the hashmap_entry to start the search from, obtained via a previous > * call to `hashmap_get` or `hashmap_get_next`. > */ > -void *hashmap_get_next(const struct hashmap *map, > +struct hashmap_entry *hashmap_get_next(const struct hashmap *map, > const struct hashmap_entry *entry); > > /* > diff --git a/name-hash.c b/name-hash.c > index f64c52bfa2..6f2779934f 100644 > --- a/name-hash.c > +++ b/name-hash.c > @@ -703,15 +703,17 @@ void adjust_dirname_case(struct index_state *istate, char *name) > struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int icase) > { > struct cache_entry *ce; > + struct hashmap_entry *ent; > > lazy_init_name_hash(istate); > > - ce = hashmap_get_from_hash(&istate->name_hash, > + ent = hashmap_get_from_hash(&istate->name_hash, > memihash(name, namelen), NULL); > - while (ce) { > + while (ent) { > + ce = container_of(ent, struct cache_entry, ent); > if (same_name(ce, name, namelen, icase)) > return ce; > - ce = hashmap_get_next(&istate->name_hash, &ce->ent); > + ent = hashmap_get_next(&istate->name_hash, ent); > } > return NULL; > } > diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c > index de2bd083b9..d85b8dc58e 100644 > --- a/t/helper/test-hashmap.c > +++ b/t/helper/test-hashmap.c > @@ -194,16 +194,18 @@ int cmd__hashmap(int argc, const char **argv) > free(entry); > > } else if (!strcmp("get", cmd) && p1) { > + struct hashmap_entry *e; > > /* lookup entry in hashmap */ > - entry = hashmap_get_from_hash(&map, hash, p1); > + e = hashmap_get_from_hash(&map, hash, p1); > > /* print result */ > - if (!entry) > + if (!e) > puts("NULL"); > - while (entry) { > + while (e) { > + entry = container_of(e, struct test_entry, ent); > puts(get_value(entry)); > - entry = hashmap_get_next(&map, &entry->ent); > + e = hashmap_get_next(&map, e); > } > > } else if (!strcmp("remove", cmd) && p1) { I didn't comment on them all, but essentially every use of container_of() here is pretty confusing with the names. Perhaps some pattern of "type_member" could be helpful, so you can use type_p = container_of(type_member, struct type, member); to be really clear about each name. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> wrote: > On 8/25/2019 10:43 PM, Eric Wong wrote: > > --- a/diff.c > > +++ b/diff.c > > @@ -1035,8 +1035,10 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o, > > { > > int i; > > char *got_match = xcalloc(1, pmb_nr); > > + struct hashmap_entry *ent = &match->ent; > > > > - for (; match; match = hashmap_get_next(hm, &match->ent)) { > > + for (; ent; ent = hashmap_get_next(hm, ent)) { > > + match = container_of(ent, struct moved_entry, ent); > > Lines like this are very difficult to parse. In this > container_of() macro, 'ent' is taking both the 'ptr' and > 'member' values. Agreed, naming is hard :< In the Linux kernel list.h implementation, there's actually list_for_each_entry, list_next_entry and a bunch of other macros which allow the caller to avoid using container_of. We only have list_first_entry, so far. We can draw inspiration from those macros by creating hashmap_get_next_entry and hashmap_for_each_entry macros which allow callers specify the type once; and there'd be no need for callers to specify the hashmap_entry pointer name at all :) Unlike the kernel, it looks like we can't rely on __typeof__ in git, but I think we can let the caller specify the type once...
diff --git a/diff.c b/diff.c index 72d3c6aa19..663b5d01f8 100644 --- a/diff.c +++ b/diff.c @@ -1035,8 +1035,10 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o, { int i; char *got_match = xcalloc(1, pmb_nr); + struct hashmap_entry *ent = &match->ent; - for (; match; match = hashmap_get_next(hm, &match->ent)) { + for (; ent; ent = hashmap_get_next(hm, ent)) { + match = container_of(ent, struct moved_entry, ent); for (i = 0; i < pmb_nr; i++) { struct moved_entry *prev = pmb[i].match; struct moved_entry *cur = (prev && prev->next_line) ? @@ -1135,8 +1137,9 @@ static void mark_color_as_moved(struct diff_options *o, for (n = 0; n < o->emitted_symbols->nr; n++) { struct hashmap *hm = NULL; + struct hashmap_entry *ent = NULL; struct moved_entry *key; - struct moved_entry *match = NULL; + struct moved_entry *match; struct emitted_diff_symbol *l = &o->emitted_symbols->buf[n]; enum diff_symbol last_symbol = 0; @@ -1144,20 +1147,20 @@ static void mark_color_as_moved(struct diff_options *o, case DIFF_SYMBOL_PLUS: hm = del_lines; key = prepare_entry(o, n); - match = hashmap_get(hm, &key->ent, NULL); + ent = hashmap_get(hm, &key->ent, NULL); free(key); break; case DIFF_SYMBOL_MINUS: hm = add_lines; key = prepare_entry(o, n); - match = hashmap_get(hm, &key->ent, NULL); + ent = hashmap_get(hm, &key->ent, NULL); free(key); break; default: flipped_block = 0; } - if (!match) { + if (!ent) { int i; adjust_last_block(o, n, block_length); @@ -1169,6 +1172,7 @@ static void mark_color_as_moved(struct diff_options *o, last_symbol = l->s; continue; } + match = container_of(ent, struct moved_entry, ent); if (o->color_moved == COLOR_MOVED_PLAIN) { last_symbol = l->s; @@ -1189,8 +1193,9 @@ 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. */ - for (; match; match = hashmap_get_next(hm, - &match->ent)) { + for (; ent; ent = hashmap_get_next(hm, ent)) { + match = container_of(ent, struct moved_entry, + 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 4670a40179..71aa240a68 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -274,7 +274,7 @@ static int find_identical_files(struct hashmap *srcs, struct diff_options *options) { int renames = 0; - + struct hashmap_entry *ent; struct diff_filespec *target = rename_dst[dst_index].two; struct file_similarity *p, *best = NULL; int i = 100, best_score = -1; @@ -282,12 +282,15 @@ static int find_identical_files(struct hashmap *srcs, /* * Find the best source match for specified destination. */ - p = hashmap_get_from_hash(srcs, + ent = hashmap_get_from_hash(srcs, hash_filespec(options->repo, target), NULL); - for (; p; p = hashmap_get_next(srcs, &p->entry)) { + for (; ent; ent = hashmap_get_next(srcs, ent)) { int score; - struct diff_filespec *source = p->filespec; + struct diff_filespec *source; + + p = container_of(ent, struct file_similarity, entry); + source = p->filespec; /* False hash collision? */ if (!oideq(&source->oid, &target->oid)) diff --git a/hashmap.c b/hashmap.c index 2dd9912e13..d6434d9ca4 100644 --- a/hashmap.c +++ b/hashmap.c @@ -192,7 +192,7 @@ void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key, return *find_entry_ptr(map, key, keydata); } -void *hashmap_get_next(const struct hashmap *map, +struct hashmap_entry *hashmap_get_next(const struct hashmap *map, const struct hashmap_entry *entry) { struct hashmap_entry *e = entry->next; diff --git a/hashmap.h b/hashmap.h index b62ee2e7b9..25643dcdc4 100644 --- a/hashmap.h +++ b/hashmap.h @@ -55,15 +55,19 @@ * * if (!strcmp("print_all_by_key", action)) { * struct long2string k, *e; + * struct hashmap_entry *ent; * hashmap_entry_init(&k->ent, memhash(&key, sizeof(long))); * k.key = key; * * flags &= ~COMPARE_VALUE; - * e = hashmap_get(&map, &k, NULL); - * if (e) { + * ent = hashmap_get(&map, &k, NULL); + * if (ent) { + * e = container_of(ent, struct long2string, ent); * printf("first: %ld %s\n", e->key, e->value); - * while ((e = hashmap_get_next(&map, e))) + * while ((ent = hashmap_get_next(&map, ent))) { + * e = container_of(ent, struct long2string, ent); * printf("found more: %ld %s\n", e->key, e->value); + * } * } * } * @@ -320,7 +324,7 @@ static inline void *hashmap_get_from_hash(const struct hashmap *map, * `entry` is the hashmap_entry to start the search from, obtained via a previous * call to `hashmap_get` or `hashmap_get_next`. */ -void *hashmap_get_next(const struct hashmap *map, +struct hashmap_entry *hashmap_get_next(const struct hashmap *map, const struct hashmap_entry *entry); /* diff --git a/name-hash.c b/name-hash.c index f64c52bfa2..6f2779934f 100644 --- a/name-hash.c +++ b/name-hash.c @@ -703,15 +703,17 @@ void adjust_dirname_case(struct index_state *istate, char *name) struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int icase) { struct cache_entry *ce; + struct hashmap_entry *ent; lazy_init_name_hash(istate); - ce = hashmap_get_from_hash(&istate->name_hash, + ent = hashmap_get_from_hash(&istate->name_hash, memihash(name, namelen), NULL); - while (ce) { + while (ent) { + ce = container_of(ent, struct cache_entry, ent); if (same_name(ce, name, namelen, icase)) return ce; - ce = hashmap_get_next(&istate->name_hash, &ce->ent); + ent = hashmap_get_next(&istate->name_hash, ent); } return NULL; } diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c index de2bd083b9..d85b8dc58e 100644 --- a/t/helper/test-hashmap.c +++ b/t/helper/test-hashmap.c @@ -194,16 +194,18 @@ int cmd__hashmap(int argc, const char **argv) free(entry); } else if (!strcmp("get", cmd) && p1) { + struct hashmap_entry *e; /* lookup entry in hashmap */ - entry = hashmap_get_from_hash(&map, hash, p1); + e = hashmap_get_from_hash(&map, hash, p1); /* print result */ - if (!entry) + if (!e) puts("NULL"); - while (entry) { + while (e) { + entry = container_of(e, struct test_entry, ent); puts(get_value(entry)); - entry = hashmap_get_next(&map, &entry->ent); + e = hashmap_get_next(&map, e); } } else if (!strcmp("remove", cmd) && p1) {
This is a step towards removing the requirement for hashmap_entry being the first field of a struct. Signed-off-by: Eric Wong <e@80x24.org> --- diff.c | 19 ++++++++++++------- diffcore-rename.c | 11 +++++++---- hashmap.c | 2 +- hashmap.h | 12 ++++++++---- name-hash.c | 8 +++++--- t/helper/test-hashmap.c | 10 ++++++---- 6 files changed, 39 insertions(+), 23 deletions(-)