* [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes
@ 2019-10-06 23:30 Eric Wong
2019-10-06 23:30 ` [PATCH v3 01/20] diff: use hashmap_entry_init on moved_entry.ent Eric Wong
` (22 more replies)
0 siblings, 23 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
v3 changes:
- use __typeof__ to avoid invalid clang warning on uninitialized var
- formatting fixes recommended by Stolee
- add Reviewed-by for Stolee
- add patch 20 to update docs to drop first member requirement
v2 here:
https://public-inbox.org/git/20190924010324.22619-1-e@80x24.org/
The following changes since commit 745f6812895b31c02b29bdfe4ae8e5498f776c26:
First batch after Git 2.23 (2019-08-22 12:41:04 -0700)
are available in the Git repository at:
https://80x24.org/git-svn.git hashmap-wip-v3
for you to fetch changes up to 4ec5200646d6b9363e64e2002eb374aa888a6c88:
hashmap_entry: remove first member requirement from docs (2019-10-06 23:05:22 +0000)
----------------------------------------------------------------
Eric Wong (20):
diff: use hashmap_entry_init on moved_entry.ent
coccicheck: detect hashmap_entry.hash assignment
packfile: use hashmap_entry in delta_base_cache_entry
hashmap_entry_init takes "struct hashmap_entry *"
hashmap_get_next takes "const struct hashmap_entry *"
hashmap_add takes "struct hashmap_entry *"
hashmap_get takes "const struct hashmap_entry *"
hashmap_remove takes "const struct hashmap_entry *"
hashmap_put takes "struct hashmap_entry *"
introduce container_of macro
hashmap_get_next returns "struct hashmap_entry *"
hashmap: use *_entry APIs to wrap container_of
hashmap_get{,_from_hash} return "struct hashmap_entry *"
hashmap_cmp_fn takes hashmap_entry params
hashmap: use *_entry APIs for iteration
hashmap: hashmap_{put,remove} return hashmap_entry *
hashmap: introduce hashmap_free_entries
OFFSETOF_VAR macro to simplify hashmap iterators
hashmap: remove type arg from hashmap_{get,put,remove}_entry
hashmap_entry: remove first member requirement from docs
attr.c | 24 ++---
blame.c | 25 ++---
builtin/describe.c | 21 +++--
builtin/difftool.c | 56 +++++++-----
builtin/fast-export.c | 15 ++-
builtin/fetch.c | 32 ++++---
config.c | 24 ++---
contrib/coccinelle/hashmap.cocci | 16 ++++
diff.c | 31 ++++---
diffcore-rename.c | 15 ++-
git-compat-util.h | 38 ++++++++
hashmap.c | 58 +++++++-----
hashmap.h | 176 +++++++++++++++++++++++++++++-------
merge-recursive.c | 87 ++++++++++--------
merge-recursive.h | 4 +-
name-hash.c | 57 ++++++------
oidmap.c | 20 ++--
oidmap.h | 6 +-
packfile.c | 22 +++--
patch-ids.c | 18 ++--
range-diff.c | 10 +-
ref-filter.c | 33 ++++---
refs.c | 25 +++--
remote.c | 21 +++--
remote.h | 2 +-
revision.c | 28 +++---
sequencer.c | 44 ++++++---
sub-process.c | 20 ++--
sub-process.h | 6 +-
submodule-config.c | 52 ++++++-----
t/helper/test-hashmap.c | 50 +++++-----
t/helper/test-lazy-init-name-hash.c | 12 +--
32 files changed, 667 insertions(+), 381 deletions(-)
create mode 100644 contrib/coccinelle/hashmap.cocci
Range-diff against v2:
1: 51ba91d2f9 ! 1: 0f530552de diff: use hashmap_entry_init on moved_entry.ent
@@ -10,6 +10,7 @@
hashmap_add callers to take "struct hashmap_entry *".
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/diff.c b/diff.c
--- a/diff.c
2: be4071d33d ! 2: d12ddb4327 coccicheck: detect hashmap_entry.hash assignment
@@ -8,6 +8,7 @@
hashmap_entry_init, instead.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/contrib/coccinelle/hashmap.cocci b/contrib/coccinelle/hashmap.cocci
new file mode 100644
3: ff40a39bad ! 3: 624b89fbb4 packfile: use hashmap_entry in delta_base_cache_entry
@@ -15,6 +15,7 @@
systems.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/packfile.c b/packfile.c
--- a/packfile.c
4: d1db0f6472 ! 4: 7512f2f2f7 hashmap_entry_init takes "struct hashmap_entry *"
@@ -8,6 +8,7 @@
safety and readability.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
@@ -212,8 +213,8 @@
* and if it is on stack, you can just let it go out of scope).
*/
-static inline void hashmap_entry_init(void *entry, unsigned int hash)
-+static inline void
-+hashmap_entry_init(struct hashmap_entry *e, unsigned int hash)
++static inline void hashmap_entry_init(struct hashmap_entry *e,
++ unsigned int hash)
{
- struct hashmap_entry *e = entry;
e->hash = hash;
5: a293445e97 ! 5: dd167a1113 hashmap_get_next takes "const struct hashmap_entry *"
@@ -6,6 +6,7 @@
now detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/diff.c b/diff.c
--- a/diff.c
6: beabdab6fe ! 6: 53f429ef6f hashmap_add takes "struct hashmap_entry *"
@@ -6,6 +6,7 @@
detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
7: 99970934c7 ! 7: 87739268dc hashmap_get takes "const struct hashmap_entry *"
@@ -6,6 +6,7 @@
now detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
8: 8148ed70eb ! 8: 22226b3bb4 hashmap_remove takes "const struct hashmap_entry *"
@@ -6,6 +6,7 @@
now detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/blame.c b/blame.c
--- a/blame.c
9: 3756dcc34e ! 9: 0b64282a1f hashmap_put takes "struct hashmap_entry *"
@@ -6,6 +6,7 @@
detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
--- a/builtin/fast-export.c
10: ff586aa43c ! 10: c2e9201838 introduce container_of macro
@@ -7,15 +7,16 @@
and chained hash tables while allowing the compiler to do
type checking.
- I intend to use this to remove the limitation of "hashmap_entry"
- being location-dependent and to allow more compile-time type
- checking.
+ Later patches will use container_of() to remove the limitation
+ of "hashmap_entry" being location-dependent. This will complete
+ the transition to compile-time type checking for the hashmap API.
This macro already exists in our source as "list_entry" in
list.h and making "list_entry" an alias to "container_of"
as the Linux kernel has done is a possibility.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/git-compat-util.h b/git-compat-util.h
--- a/git-compat-util.h
11: 931356f061 ! 11: c709e607b1 hashmap_get_next returns "struct hashmap_entry *"
@@ -6,6 +6,7 @@
hashmap_entry being the first field of a struct.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/diff.c b/diff.c
--- a/diff.c
@@ -14,10 +15,10 @@
{
int i;
char *got_match = xcalloc(1, pmb_nr);
-+ struct hashmap_entry *ent = &match->ent;
++ struct hashmap_entry *ent;
- for (; match; match = hashmap_get_next(hm, &match->ent)) {
-+ for (; ent; ent = hashmap_get_next(hm, ent)) {
++ for (ent = &match->ent; 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;
12: a748b9b0c4 ! 12: 27ba68a7b8 hashmap: use *_entry APIs to wrap container_of
@@ -11,6 +11,7 @@
extra parameter to specify the type.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/diff.c b/diff.c
--- a/diff.c
@@ -19,9 +20,9 @@
{
int i;
char *got_match = xcalloc(1, pmb_nr);
-- struct hashmap_entry *ent = &match->ent;
+- struct hashmap_entry *ent;
-- for (; ent; ent = hashmap_get_next(hm, ent)) {
+- for (ent = &match->ent; ent; ent = hashmap_get_next(hm, ent)) {
- match = container_of(ent, struct moved_entry, ent);
+ hashmap_for_each_entry_from(hm, match, struct moved_entry, ent) {
for (i = 0; i < pmb_nr; i++) {
@@ -125,7 +126,6 @@
#define container_of(ptr, type, member) \
((type *) ((char *)(ptr) - offsetof(type, member)))
-+
+/*
+ * helper function for `container_of_or_null' to avoid multiple
+ * evaluation of @ptr
13: e8faa5e12e ! 13: 222b1f07f3 hashmap_get{,_from_hash} return "struct hashmap_entry *"
@@ -9,6 +9,7 @@
hashmap_entry being the first field in a struct.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
@@ -152,11 +153,13 @@
}
-void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
-+struct hashmap_entry *
-+hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata)
+- const void *keydata)
++struct hashmap_entry *hashmap_get(const struct hashmap *map,
++ const struct hashmap_entry *key,
++ const void *keydata)
{
return *find_entry_ptr(map, key, keydata);
+ }
@@
/* lookup interned string in pool */
hashmap_entry_init(&key.ent, memhash(data, len));
@@ -175,21 +178,27 @@
* to `hashmap_cmp_fn` to decide whether the entry matches the key.
*/
-void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
-+struct hashmap_entry *
-+hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata);
+- const void *keydata);
++struct hashmap_entry *hashmap_get(const struct hashmap *map,
++ const struct hashmap_entry *key,
++ const void *keydata);
/*
+ * Returns the hashmap entry for the specified hash code and key data,
@@
* `entry_or_key` parameter of `hashmap_cmp_fn` points to a hashmap_entry
* structure that should not be used in the comparison.
*/
-static inline void *hashmap_get_from_hash(const struct hashmap *map,
-+static inline struct hashmap_entry *
-+hashmap_get_from_hash(const struct hashmap *map,
- unsigned int hash,
- const void *keydata)
+- unsigned int hash,
+- const void *keydata)
++static inline struct hashmap_entry *hashmap_get_from_hash(
++ const struct hashmap *map,
++ unsigned int hash,
++ const void *keydata)
{
+ struct hashmap_entry key;
+ hashmap_entry_init(&key, hash);
diff --git a/merge-recursive.c b/merge-recursive.c
--- a/merge-recursive.c
14: f84bd96b8e ! 14: c789f98c81 hashmap_cmp_fn takes hashmap_entry params
@@ -6,6 +6,7 @@
being the first member of a struct.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
15: 23fbd888d9 ! 15: 57ae5b5142 hashmap: use *_entry APIs for iteration
@@ -7,6 +7,7 @@
by compilers lacking __typeof__ support.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
16: 6908364381 ! 16: f1e35ba094 hashmap: hashmap_{put,remove} return hashmap_entry *
@@ -6,6 +6,7 @@
to simplify most callers.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/hashmap.c b/hashmap.c
--- a/hashmap.c
@@ -15,18 +16,20 @@
}
-void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
-+struct hashmap_entry *
-+hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata)
+- const void *keydata)
++struct hashmap_entry *hashmap_remove(struct hashmap *map,
++ const struct hashmap_entry *key,
++ const void *keydata)
{
struct hashmap_entry *old;
+ struct hashmap_entry **e = find_entry_ptr(map, key, keydata);
@@
return old;
}
-void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry)
-+struct hashmap_entry *
-+hashmap_put(struct hashmap *map, struct hashmap_entry *entry)
++struct hashmap_entry *hashmap_put(struct hashmap *map,
++ struct hashmap_entry *entry)
{
struct hashmap_entry *old = hashmap_remove(map, entry, NULL);
hashmap_add(map, entry);
@@ -39,8 +42,8 @@
* Returns the replaced entry, or NULL if not found (i.e. the entry was added).
*/
-void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
-+struct hashmap_entry *
-+hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
++struct hashmap_entry *hashmap_put(struct hashmap *map,
++ struct hashmap_entry *entry);
+
+#define hashmap_put_entry(map, keyvar, type, member) \
+ container_of_or_null(hashmap_put(map, &(keyvar)->member), type, member)
@@ -52,17 +55,17 @@
* Argument explanation is the same as in `hashmap_get`.
*/
-void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
-+struct hashmap_entry *
-+hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata);
-
+- const void *keydata);
++struct hashmap_entry *hashmap_remove(struct hashmap *map,
++ const struct hashmap_entry *key,
++ const void *keydata);
++
+#define hashmap_remove_entry(map, keyvar, keydata, type, member) \
+ container_of_or_null(hashmap_remove(map, &(keyvar)->member, keydata), \
+ type, member)
-+
+
/*
* Returns the `bucket` an entry is stored in.
- * Useful for multithreaded read access.
diff --git a/range-diff.c b/range-diff.c
--- a/range-diff.c
17: 150944128b ! 17: adc7cb5516 hashmap: introduce hashmap_free_entries
@@ -12,6 +12,7 @@
the hashmap itself.
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/blame.c b/blame.c
--- a/blame.c
18: de212aa6d6 ! 18: 1a68e1645d OFFSETOF_VAR macro to simplify hashmap iterators
@@ -5,7 +5,7 @@
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.
+ pointer arithmetic for compilers without `__typeof__'.
This allows us to simplify usage of hashmap iterator macros
by not having to specify a type when a pointer of that type
@@ -16,7 +16,10 @@
trouble of using container_of/list_entry macros and without
relying on non-portable `__typeof__'.
+ v3: use `__typeof__' to avoid clang warnings
+
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
@@ -141,11 +144,17 @@
(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()
++ * like offsetof(), but takes a pointer to a a variable of type which
++ * contains @member, instead of a specified type.
++ * @ptr is subject to multiple evaluation since we can't rely on __typeof__
++ * everywhere.
+ */
++#if defined(__GNUC__) /* clang sets this, too */
++#define OFFSETOF_VAR(ptr, member) offsetof(__typeof__(*ptr), member)
++#else /* !__GNUC__ */
+#define OFFSETOF_VAR(ptr, member) \
+ ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))
++#endif /* !__GNUC__ */
+
#endif
19: 212a596edd ! 19: fe02b9f839 hashmap: remove type arg from hashmap_{get,put,remove}_entry
@@ -10,6 +10,7 @@
sequential as they are used as: `keyvar->member'
Signed-off-by: Eric Wong <e@80x24.org>
+ Reviewed-by: Derrick Stolee <stolee@gmail.com>
diff --git a/attr.c b/attr.c
--- a/attr.c
@@ -147,8 +148,8 @@
* printf("first: %ld %s\n", e->key, e->value);
* while ((e = hashmap_get_next_entry(&map, e,
@@
- struct hashmap_entry *
- hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
+ struct hashmap_entry *hashmap_put(struct hashmap *map,
+ struct hashmap_entry *entry);
-#define hashmap_put_entry(map, keyvar, type, member) \
- container_of_or_null(hashmap_put(map, &(keyvar)->member), type, member)
@@ -167,8 +168,8 @@
/*
* Removes a hashmap entry matching the specified key. If the hashmap contains
@@
- hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata);
+ const struct hashmap_entry *key,
+ const void *keydata);
-#define hashmap_remove_entry(map, keyvar, keydata, type, member) \
- container_of_or_null(hashmap_remove(map, &(keyvar)->member, keydata), \
-: ---------- > 20: 4ec5200646 hashmap_entry: remove first member requirement from docs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 01/20] diff: use hashmap_entry_init on moved_entry.ent
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 02/20] coccicheck: detect hashmap_entry.hash assignment Eric Wong
` (21 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Otherwise, the hashmap_entry.next field appears to remain
uninitialized, which can lead to problems when
add_lines_to_move_detection calls hashmap_add.
I found this through manual inspection when converting
hashmap_add callers to take "struct hashmap_entry *".
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
diff.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index efe42b341a..02491ee684 100644
--- a/diff.c
+++ b/diff.c
@@ -964,8 +964,9 @@ static struct moved_entry *prepare_entry(struct diff_options *o,
struct moved_entry *ret = xmalloc(sizeof(*ret));
struct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];
unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
+ unsigned int hash = xdiff_hash_string(l->line, l->len, flags);
- ret->ent.hash = xdiff_hash_string(l->line, l->len, flags);
+ hashmap_entry_init(&ret->ent, hash);
ret->es = l;
ret->next_line = NULL;
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 02/20] coccicheck: detect hashmap_entry.hash assignment
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
2019-10-06 23:30 ` [PATCH v3 01/20] diff: use hashmap_entry_init on moved_entry.ent Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 03/20] packfile: use hashmap_entry in delta_base_cache_entry Eric Wong
` (20 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Assigning hashmap_entry.hash manually leaves hashmap_entry.next
uninitialized, which can be dangerous once the hashmap_entry is
inserted into a hashmap. Detect those assignments and use
hashmap_entry_init, instead.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
contrib/coccinelle/hashmap.cocci | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 contrib/coccinelle/hashmap.cocci
diff --git a/contrib/coccinelle/hashmap.cocci b/contrib/coccinelle/hashmap.cocci
new file mode 100644
index 0000000000..d69e120ccf
--- /dev/null
+++ b/contrib/coccinelle/hashmap.cocci
@@ -0,0 +1,16 @@
+@ hashmap_entry_init_usage @
+expression E;
+struct hashmap_entry HME;
+@@
+- HME.hash = E;
++ hashmap_entry_init(&HME, E);
+
+@@
+identifier f !~ "^hashmap_entry_init$";
+expression E;
+struct hashmap_entry *HMEP;
+@@
+ f(...) {<...
+- HMEP->hash = E;
++ hashmap_entry_init(HMEP, E);
+ ...>}
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 03/20] packfile: use hashmap_entry in delta_base_cache_entry
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
2019-10-06 23:30 ` [PATCH v3 01/20] diff: use hashmap_entry_init on moved_entry.ent Eric Wong
2019-10-06 23:30 ` [PATCH v3 02/20] coccicheck: detect hashmap_entry.hash assignment Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 04/20] hashmap_entry_init takes "struct hashmap_entry *" Eric Wong
` (19 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
This hashmap_entry_init function is intended to take a
hashmap_entry struct pointer, not a hashmap struct pointer.
This was not noticed because hashmap_entry_init takes a "void *"
arg instead of "struct hashmap_entry *", and the hashmap struct
is larger and can be cast into a hashmap_entry struct without
data corruption.
This has the beneficial side effect of reducing the size of
a delta_base_cache_entry from 104 bytes to 72 bytes on 64-bit
systems.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
packfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/packfile.c b/packfile.c
index fc43a6c52c..37fe0b73a6 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1361,7 +1361,7 @@ struct delta_base_cache_key {
};
struct delta_base_cache_entry {
- struct hashmap hash;
+ struct hashmap_entry ent;
struct delta_base_cache_key key;
struct list_head lru;
void *data;
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 04/20] hashmap_entry_init takes "struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (2 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 03/20] packfile: use hashmap_entry in delta_base_cache_entry Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 05/20] hashmap_get_next takes "const struct " Eric Wong
` (18 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
C compilers do type checking to make life easier for us. So
rely on that and update all hashmap_entry_init callers to take
"struct hashmap_entry *" to avoid future bugs while improving
safety and readability.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
attr.c | 4 ++--
blame.c | 2 +-
builtin/describe.c | 2 +-
builtin/difftool.c | 6 +++---
builtin/fast-export.c | 2 +-
builtin/fetch.c | 2 +-
config.c | 4 ++--
diffcore-rename.c | 2 +-
hashmap.c | 4 ++--
hashmap.h | 12 ++++++------
merge-recursive.c | 13 +++++++------
name-hash.c | 10 +++++-----
packfile.c | 2 +-
patch-ids.c | 2 +-
range-diff.c | 4 ++--
ref-filter.c | 3 ++-
refs.c | 2 +-
remote.c | 2 +-
revision.c | 4 ++--
sequencer.c | 5 +++--
sub-process.c | 4 ++--
submodule-config.c | 10 +++++-----
t/helper/test-hashmap.c | 6 +++---
23 files changed, 55 insertions(+), 52 deletions(-)
diff --git a/attr.c b/attr.c
index 93dc16b59c..a8be7a7b4f 100644
--- a/attr.c
+++ b/attr.c
@@ -98,7 +98,7 @@ static void *attr_hashmap_get(struct attr_hashmap *map,
if (!map->map.tablesize)
attr_hashmap_init(map);
- hashmap_entry_init(&k, memhash(key, keylen));
+ hashmap_entry_init(&k.ent, memhash(key, keylen));
k.key = key;
k.keylen = keylen;
e = hashmap_get(&map->map, &k, NULL);
@@ -117,7 +117,7 @@ static void attr_hashmap_add(struct attr_hashmap *map,
attr_hashmap_init(map);
e = xmalloc(sizeof(struct attr_hash_entry));
- hashmap_entry_init(e, memhash(key, keylen));
+ hashmap_entry_init(&e->ent, memhash(key, keylen));
e->key = key;
e->keylen = keylen;
e->value = value;
diff --git a/blame.c b/blame.c
index 36a2e7ef11..46059410cd 100644
--- a/blame.c
+++ b/blame.c
@@ -417,7 +417,7 @@ static void get_fingerprint(struct fingerprint *result,
/* Ignore whitespace pairs */
if (hash == 0)
continue;
- hashmap_entry_init(entry, hash);
+ hashmap_entry_init(&entry->entry, hash);
found_entry = hashmap_get(&result->map, entry, NULL);
if (found_entry) {
diff --git a/builtin/describe.c b/builtin/describe.c
index 200154297d..596ddf89a5 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -123,7 +123,7 @@ static void add_to_known_names(const char *path,
if (!e) {
e = xmalloc(sizeof(struct commit_name));
oidcpy(&e->peeled, peeled);
- hashmap_entry_init(e, oidhash(peeled));
+ hashmap_entry_init(&e->entry, oidhash(peeled));
hashmap_add(&names, e);
e->path = NULL;
}
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 16eb8b70ea..98ffc04c61 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -161,7 +161,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
struct pair_entry *e, *existing;
FLEX_ALLOC_STR(e, path, path);
- hashmap_entry_init(e, strhash(path));
+ hashmap_entry_init(&e->entry, strhash(path));
existing = hashmap_get(map, e, NULL);
if (existing) {
free(e);
@@ -234,7 +234,7 @@ static void changed_files(struct hashmap *result, const char *index_path,
while (!strbuf_getline_nul(&buf, fp)) {
struct path_entry *entry;
FLEX_ALLOC_STR(entry, path, buf.buf);
- hashmap_entry_init(entry, strhash(buf.buf));
+ hashmap_entry_init(&entry->entry, strhash(buf.buf));
hashmap_add(result, entry);
}
fclose(fp);
@@ -461,7 +461,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
/* Avoid duplicate working_tree entries */
FLEX_ALLOC_STR(entry, path, dst_path);
- hashmap_entry_init(entry, strhash(dst_path));
+ hashmap_entry_init(&entry->entry, strhash(dst_path));
if (hashmap_get(&working_tree_dups, entry, NULL)) {
free(entry);
continue;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index f541f55d33..287dbd24a3 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -148,7 +148,7 @@ static const void *anonymize_mem(struct hashmap *map,
if (!map->cmpfn)
hashmap_init(map, anonymized_entry_cmp, NULL, 0);
- hashmap_entry_init(&key, memhash(orig, *len));
+ hashmap_entry_init(&key.hash, memhash(orig, *len));
key.orig = orig;
key.orig_len = *len;
ret = hashmap_get(map, &key, NULL);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 717dd14e89..b7d70eee70 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -276,7 +276,7 @@ static struct refname_hash_entry *refname_hash_add(struct hashmap *map,
size_t len = strlen(refname);
FLEX_ALLOC_MEM(ent, refname, refname, len);
- hashmap_entry_init(ent, strhash(refname));
+ hashmap_entry_init(&ent->ent, strhash(refname));
oidcpy(&ent->oid, oid);
hashmap_add(map, ent);
return ent;
diff --git a/config.c b/config.c
index 3900e4947b..08d866e7de 100644
--- a/config.c
+++ b/config.c
@@ -1861,7 +1861,7 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
if (git_config_parse_key(key, &normalized_key, NULL))
return NULL;
- hashmap_entry_init(&k, strhash(normalized_key));
+ hashmap_entry_init(&k.ent, strhash(normalized_key));
k.key = normalized_key;
found_entry = hashmap_get(&cs->config_hash, &k, NULL);
free(normalized_key);
@@ -1882,7 +1882,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
*/
if (!e) {
e = xmalloc(sizeof(*e));
- hashmap_entry_init(e, strhash(key));
+ hashmap_entry_init(&e->ent, strhash(key));
e->key = xstrdup(key);
string_list_init(&e->value_list, 1);
hashmap_add(&cs->config_hash, e);
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 9624864858..44a3ab1e31 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -329,7 +329,7 @@ static void insert_file_table(struct repository *r,
entry->index = index;
entry->filespec = filespec;
- hashmap_entry_init(entry, hash_filespec(r, filespec));
+ hashmap_entry_init(&entry->entry, hash_filespec(r, filespec));
hashmap_add(table, entry);
}
diff --git a/hashmap.c b/hashmap.c
index d42f01ff5a..6818c65174 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -293,13 +293,13 @@ const void *memintern(const void *data, size_t len)
hashmap_init(&map, (hashmap_cmp_fn) pool_entry_cmp, NULL, 0);
/* lookup interned string in pool */
- hashmap_entry_init(&key, memhash(data, len));
+ hashmap_entry_init(&key.ent, memhash(data, len));
key.len = len;
e = hashmap_get(&map, &key, data);
if (!e) {
/* not found: create it */
FLEX_ALLOC_MEM(e, data, data, len);
- hashmap_entry_init(e, key.ent.hash);
+ hashmap_entry_init(&e->ent, key.ent.hash);
e->len = len;
hashmap_add(&map, e);
}
diff --git a/hashmap.h b/hashmap.h
index 8424911566..54b0b8c698 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -48,14 +48,14 @@
* if (!strcmp("add", action)) {
* struct long2string *e;
* FLEX_ALLOC_STR(e, value, value);
- * hashmap_entry_init(e, memhash(&key, sizeof(long)));
+ * hashmap_entry_init(&e->ent, memhash(&key, sizeof(long)));
* e->key = key;
* hashmap_add(&map, e);
* }
*
* if (!strcmp("print_all_by_key", action)) {
* struct long2string k, *e;
- * hashmap_entry_init(&k, memhash(&key, sizeof(long)));
+ * hashmap_entry_init(&k->ent, memhash(&key, sizeof(long)));
* k.key = key;
*
* flags &= ~COMPARE_VALUE;
@@ -70,7 +70,7 @@
* if (!strcmp("has_exact_match", action)) {
* struct long2string *e;
* FLEX_ALLOC_STR(e, value, value);
- * hashmap_entry_init(e, memhash(&key, sizeof(long)));
+ * hashmap_entry_init(&e->ent, memhash(&key, sizeof(long)));
* e->key = key;
*
* flags |= COMPARE_VALUE;
@@ -80,7 +80,7 @@
*
* if (!strcmp("has_exact_match_no_heap_alloc", action)) {
* struct long2string k;
- * hashmap_entry_init(&k, memhash(&key, sizeof(long)));
+ * hashmap_entry_init(&k->ent, memhash(&key, sizeof(long)));
* k.key = key;
*
* flags |= COMPARE_VALUE;
@@ -244,9 +244,9 @@ void hashmap_free(struct hashmap *map, int free_entries);
* your structure was allocated with xmalloc(), you can just free(3) it,
* and if it is on stack, you can just let it go out of scope).
*/
-static inline void hashmap_entry_init(void *entry, unsigned int hash)
+static inline void hashmap_entry_init(struct hashmap_entry *e,
+ unsigned int hash)
{
- struct hashmap_entry *e = entry;
e->hash = hash;
e->next = NULL;
}
diff --git a/merge-recursive.c b/merge-recursive.c
index 6b812d67e3..6bc4f14ff4 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -61,7 +61,7 @@ static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
if (dir == NULL)
return NULL;
- hashmap_entry_init(&key, strhash(dir));
+ hashmap_entry_init(&key.ent, strhash(dir));
key.dir = dir;
return hashmap_get(hashmap, &key, NULL);
}
@@ -85,7 +85,7 @@ static void dir_rename_init(struct hashmap *map)
static void dir_rename_entry_init(struct dir_rename_entry *entry,
char *directory)
{
- hashmap_entry_init(entry, strhash(directory));
+ hashmap_entry_init(&entry->ent, strhash(directory));
entry->dir = directory;
entry->non_unique_new_dir = 0;
strbuf_init(&entry->new_dir, 0);
@@ -97,7 +97,7 @@ static struct collision_entry *collision_find_entry(struct hashmap *hashmap,
{
struct collision_entry key;
- hashmap_entry_init(&key, strhash(target_file));
+ hashmap_entry_init(&key.ent, strhash(target_file));
key.target_file = target_file;
return hashmap_get(hashmap, &key, NULL);
}
@@ -454,7 +454,7 @@ static int save_files_dirs(const struct object_id *oid,
strbuf_addstr(base, path);
FLEX_ALLOC_MEM(entry, path, base->buf, base->len);
- hashmap_entry_init(entry, path_hash(entry->path));
+ hashmap_entry_init(&entry->e, path_hash(entry->path));
hashmap_add(&opt->current_file_dir_set, entry);
strbuf_setlen(base, baselen);
@@ -731,7 +731,7 @@ static char *unique_path(struct merge_options *opt, const char *path, const char
}
FLEX_ALLOC_MEM(entry, path, newpath.buf, newpath.len);
- hashmap_entry_init(entry, path_hash(entry->path));
+ hashmap_entry_init(&entry->e, path_hash(entry->path));
hashmap_add(&opt->current_file_dir_set, entry);
return strbuf_detach(&newpath, NULL);
}
@@ -2358,7 +2358,8 @@ static void compute_collisions(struct hashmap *collisions,
if (!collision_ent) {
collision_ent = xcalloc(1,
sizeof(struct collision_entry));
- hashmap_entry_init(collision_ent, strhash(new_path));
+ hashmap_entry_init(&collision_ent->ent,
+ strhash(new_path));
hashmap_put(collisions, collision_ent);
collision_ent->target_file = new_path;
} else {
diff --git a/name-hash.c b/name-hash.c
index 695908609f..1ce1417f7e 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -33,7 +33,7 @@ static struct dir_entry *find_dir_entry__hash(struct index_state *istate,
const char *name, unsigned int namelen, unsigned int hash)
{
struct dir_entry key;
- hashmap_entry_init(&key, hash);
+ hashmap_entry_init(&key.ent, hash);
key.namelen = namelen;
return hashmap_get(&istate->dir_hash, &key, name);
}
@@ -68,7 +68,7 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,
if (!dir) {
/* not found, create it and add to hash table */
FLEX_ALLOC_MEM(dir, name, ce->name, namelen);
- hashmap_entry_init(dir, memihash(ce->name, namelen));
+ hashmap_entry_init(&dir->ent, memihash(ce->name, namelen));
dir->namelen = namelen;
hashmap_add(&istate->dir_hash, dir);
@@ -106,7 +106,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
if (ce->ce_flags & CE_HASHED)
return;
ce->ce_flags |= CE_HASHED;
- hashmap_entry_init(ce, memihash(ce->name, ce_namelen(ce)));
+ hashmap_entry_init(&ce->ent, memihash(ce->name, ce_namelen(ce)));
hashmap_add(&istate->name_hash, ce);
if (ignore_case)
@@ -280,7 +280,7 @@ static struct dir_entry *hash_dir_entry_with_parent_and_prefix(
dir = find_dir_entry__hash(istate, prefix->buf, prefix->len, hash);
if (!dir) {
FLEX_ALLOC_MEM(dir, name, prefix->buf, prefix->len);
- hashmap_entry_init(dir, hash);
+ hashmap_entry_init(&dir->ent, hash);
dir->namelen = prefix->len;
dir->parent = parent;
hashmap_add(&istate->dir_hash, dir);
@@ -472,7 +472,7 @@ static void *lazy_name_thread_proc(void *_data)
for (k = 0; k < d->istate->cache_nr; k++) {
struct cache_entry *ce_k = d->istate->cache[k];
ce_k->ce_flags |= CE_HASHED;
- hashmap_entry_init(ce_k, d->lazy_entries[k].hash_name);
+ hashmap_entry_init(&ce_k->ent, d->lazy_entries[k].hash_name);
hashmap_add(&d->istate->name_hash, ce_k);
}
diff --git a/packfile.c b/packfile.c
index 37fe0b73a6..96535eb86b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1487,7 +1487,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
if (!delta_base_cache.cmpfn)
hashmap_init(&delta_base_cache, delta_base_cache_hash_cmp, NULL, 0);
- hashmap_entry_init(ent, pack_entry_hash(p, base_offset));
+ hashmap_entry_init(&ent->ent, pack_entry_hash(p, base_offset));
hashmap_add(&delta_base_cache, ent);
}
diff --git a/patch-ids.c b/patch-ids.c
index e8c150d0c9..a2da711678 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -83,7 +83,7 @@ static int init_patch_id_entry(struct patch_id *patch,
if (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1, 0))
return -1;
- hashmap_entry_init(patch, oidhash(&header_only_patch_id));
+ hashmap_entry_init(&patch->ent, oidhash(&header_only_patch_id));
return 0;
}
diff --git a/range-diff.c b/range-diff.c
index ba1e9a4265..32b29f9594 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -217,7 +217,7 @@ static void find_exact_matches(struct string_list *a, struct string_list *b)
util->i = i;
util->patch = a->items[i].string;
util->diff = util->patch + util->diff_offset;
- hashmap_entry_init(util, strhash(util->diff));
+ hashmap_entry_init(&util->e, strhash(util->diff));
hashmap_add(&map, util);
}
@@ -228,7 +228,7 @@ static void find_exact_matches(struct string_list *a, struct string_list *b)
util->i = i;
util->patch = b->items[i].string;
util->diff = util->patch + util->diff_offset;
- hashmap_entry_init(util, strhash(util->diff));
+ hashmap_entry_init(&util->e, strhash(util->diff));
other = hashmap_remove(&map, util, NULL);
if (other) {
if (other->matching >= 0)
diff --git a/ref-filter.c b/ref-filter.c
index f27cfc8c3e..206014c93d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1565,7 +1565,8 @@ static void populate_worktree_map(struct hashmap *map, struct worktree **worktre
struct ref_to_worktree_entry *entry;
entry = xmalloc(sizeof(*entry));
entry->wt = worktrees[i];
- hashmap_entry_init(entry, strhash(worktrees[i]->head_ref));
+ hashmap_entry_init(&entry->ent,
+ strhash(worktrees[i]->head_ref));
hashmap_add(map, entry);
}
diff --git a/refs.c b/refs.c
index cd297ee4bd..c43ec59c6e 100644
--- a/refs.c
+++ b/refs.c
@@ -1796,7 +1796,7 @@ static struct ref_store_hash_entry *alloc_ref_store_hash_entry(
struct ref_store_hash_entry *entry;
FLEX_ALLOC_STR(entry, name, name);
- hashmap_entry_init(entry, strhash(name));
+ hashmap_entry_init(&entry->ent, strhash(name));
entry->refs = refs;
return entry;
}
diff --git a/remote.c b/remote.c
index e50f7602ed..bd81cb71bc 100644
--- a/remote.c
+++ b/remote.c
@@ -158,7 +158,7 @@ static struct remote *make_remote(const char *name, int len)
ALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);
remotes[remotes_nr++] = ret;
- hashmap_entry_init(ret, lookup_entry.hash);
+ hashmap_entry_init(&ret->ent, lookup_entry.hash);
replaced = hashmap_put(&remotes_hash, ret);
assert(replaced == NULL); /* no previous entry overwritten */
return ret;
diff --git a/revision.c b/revision.c
index 07412297f0..3461c78883 100644
--- a/revision.c
+++ b/revision.c
@@ -141,7 +141,7 @@ static void paths_and_oids_insert(struct hashmap *map,
struct path_and_oids_entry key;
struct path_and_oids_entry *entry;
- hashmap_entry_init(&key, hash);
+ hashmap_entry_init(&key.ent, hash);
/* use a shallow copy for the lookup */
key.path = (char *)path;
@@ -149,7 +149,7 @@ static void paths_and_oids_insert(struct hashmap *map,
if (!(entry = (struct path_and_oids_entry *)hashmap_get(map, &key, NULL))) {
entry = xcalloc(1, sizeof(struct path_and_oids_entry));
- hashmap_entry_init(entry, hash);
+ hashmap_entry_init(&entry->ent, hash);
entry->path = xstrdup(key.path);
oidset_init(&entry->trees, 16);
hashmap_put(map, entry);
diff --git a/sequencer.c b/sequencer.c
index 34ebf8ed94..1140cdf526 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4538,7 +4538,7 @@ static const char *label_oid(struct object_id *oid, const char *label,
}
FLEX_ALLOC_STR(labels_entry, label, label);
- hashmap_entry_init(labels_entry, strihash(label));
+ hashmap_entry_init(&labels_entry->entry, strihash(label));
hashmap_add(&state->labels, labels_entry);
FLEX_ALLOC_STR(string_entry, string, label);
@@ -5252,7 +5252,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
strhash(subject), subject)) {
FLEX_ALLOC_MEM(entry, subject, subject, subject_len);
entry->i = i;
- hashmap_entry_init(entry, strhash(entry->subject));
+ hashmap_entry_init(&entry->entry,
+ strhash(entry->subject));
hashmap_put(&subject2item, entry);
}
}
diff --git a/sub-process.c b/sub-process.c
index 3f4af93555..9847dad6fc 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -20,7 +20,7 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch
{
struct subprocess_entry key;
- hashmap_entry_init(&key, strhash(cmd));
+ hashmap_entry_init(&key.ent, strhash(cmd));
key.cmd = cmd;
return hashmap_get(hashmap, &key, NULL);
}
@@ -96,7 +96,7 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
return err;
}
- hashmap_entry_init(entry, strhash(cmd));
+ hashmap_entry_init(&entry->ent, strhash(cmd));
err = startfn(entry);
if (err) {
diff --git a/submodule-config.c b/submodule-config.c
index 4264ee216f..4aa02e280e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -123,7 +123,7 @@ static void cache_put_path(struct submodule_cache *cache,
unsigned int hash = hash_oid_string(&submodule->gitmodules_oid,
submodule->path);
struct submodule_entry *e = xmalloc(sizeof(*e));
- hashmap_entry_init(e, hash);
+ hashmap_entry_init(&e->ent, hash);
e->config = submodule;
hashmap_put(&cache->for_path, e);
}
@@ -135,7 +135,7 @@ static void cache_remove_path(struct submodule_cache *cache,
submodule->path);
struct submodule_entry e;
struct submodule_entry *removed;
- hashmap_entry_init(&e, hash);
+ hashmap_entry_init(&e.ent, hash);
e.config = submodule;
removed = hashmap_remove(&cache->for_path, &e, NULL);
free(removed);
@@ -147,7 +147,7 @@ static void cache_add(struct submodule_cache *cache,
unsigned int hash = hash_oid_string(&submodule->gitmodules_oid,
submodule->name);
struct submodule_entry *e = xmalloc(sizeof(*e));
- hashmap_entry_init(e, hash);
+ hashmap_entry_init(&e->ent, hash);
e->config = submodule;
hashmap_add(&cache->for_name, e);
}
@@ -163,7 +163,7 @@ static const struct submodule *cache_lookup_path(struct submodule_cache *cache,
oidcpy(&key_config.gitmodules_oid, gitmodules_oid);
key_config.path = path;
- hashmap_entry_init(&key, hash);
+ hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
entry = hashmap_get(&cache->for_path, &key, NULL);
@@ -183,7 +183,7 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
oidcpy(&key_config.gitmodules_oid, gitmodules_oid);
key_config.name = name;
- hashmap_entry_init(&key, hash);
+ hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
entry = hashmap_get(&cache->for_name, &key, NULL);
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index aaf17b0ddf..0c9fd7c996 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -37,7 +37,7 @@ static struct test_entry *alloc_test_entry(unsigned int hash,
size_t klen = strlen(key);
size_t vlen = strlen(value);
struct test_entry *entry = xmalloc(st_add4(sizeof(*entry), klen, vlen, 2));
- hashmap_entry_init(entry, hash);
+ hashmap_entry_init(&entry->ent, hash);
memcpy(entry->key, key, klen + 1);
memcpy(entry->key + klen + 1, value, vlen + 1);
return entry;
@@ -103,7 +103,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
/* add entries */
for (i = 0; i < TEST_SIZE; i++) {
- hashmap_entry_init(entries[i], hashes[i]);
+ hashmap_entry_init(&entries[i]->ent, hashes[i]);
hashmap_add(&map, entries[i]);
}
@@ -116,7 +116,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
/* fill the map (sparsely if specified) */
j = (method & TEST_SPARSE) ? TEST_SIZE / 10 : TEST_SIZE;
for (i = 0; i < j; i++) {
- hashmap_entry_init(entries[i], hashes[i]);
+ hashmap_entry_init(&entries[i]->ent, hashes[i]);
hashmap_add(&map, entries[i]);
}
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 05/20] hashmap_get_next takes "const struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (3 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 04/20] hashmap_entry_init takes "struct hashmap_entry *" Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 06/20] hashmap_add takes "struct " Eric Wong
` (17 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
This is less error-prone than "const void *" as the compiler
now detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
diff.c | 5 +++--
diffcore-rename.c | 2 +-
hashmap.c | 5 +++--
hashmap.h | 3 ++-
name-hash.c | 2 +-
t/helper/test-hashmap.c | 2 +-
6 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/diff.c b/diff.c
index 02491ee684..1168f0cbb9 100644
--- a/diff.c
+++ b/diff.c
@@ -1036,7 +1036,7 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,
int i;
char *got_match = xcalloc(1, pmb_nr);
- for (; match; match = hashmap_get_next(hm, match)) {
+ for (; match; match = hashmap_get_next(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) ?
@@ -1189,7 +1189,8 @@ 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)) {
+ for (; match; match = hashmap_get_next(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 44a3ab1e31..2a1449013b 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -285,7 +285,7 @@ static int find_identical_files(struct hashmap *srcs,
p = hashmap_get_from_hash(srcs,
hash_filespec(options->repo, target),
NULL);
- for (; p; p = hashmap_get_next(srcs, p)) {
+ for (; p; p = hashmap_get_next(srcs, &p->entry)) {
int score;
struct diff_filespec *source = p->filespec;
diff --git a/hashmap.c b/hashmap.c
index 6818c65174..c1de40eea0 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -191,9 +191,10 @@ void *hashmap_get(const struct hashmap *map, const void *key, const void *keydat
return *find_entry_ptr(map, key, keydata);
}
-void *hashmap_get_next(const struct hashmap *map, const void *entry)
+void *hashmap_get_next(const struct hashmap *map,
+ const struct hashmap_entry *entry)
{
- struct hashmap_entry *e = ((struct hashmap_entry *) entry)->next;
+ struct hashmap_entry *e = entry->next;
for (; e; e = e->next)
if (entry_equals(map, entry, e, NULL))
return e;
diff --git a/hashmap.h b/hashmap.h
index 54b0b8c698..93fb9599ca 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -318,7 +318,8 @@ 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, const void *entry);
+void *hashmap_get_next(const struct hashmap *map,
+ const struct hashmap_entry *entry);
/*
* Adds a hashmap entry. This allows to add duplicate entries (i.e.
diff --git a/name-hash.c b/name-hash.c
index 1ce1417f7e..4d84326c58 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -710,7 +710,7 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na
while (ce) {
if (same_name(ce, name, namelen, icase))
return ce;
- ce = hashmap_get_next(&istate->name_hash, ce);
+ ce = hashmap_get_next(&istate->name_hash, &ce->ent);
}
return NULL;
}
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 0c9fd7c996..bf063a2521 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -203,7 +203,7 @@ int cmd__hashmap(int argc, const char **argv)
puts("NULL");
while (entry) {
puts(get_value(entry));
- entry = hashmap_get_next(&map, entry);
+ entry = hashmap_get_next(&map, &entry->ent);
}
} else if (!strcmp("remove", cmd) && p1) {
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 06/20] hashmap_add takes "struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (4 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 05/20] hashmap_get_next takes "const struct " Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 07/20] hashmap_get takes "const struct " Eric Wong
` (16 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
This is less error-prone than "void *" as the compiler now
detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
attr.c | 2 +-
blame.c | 2 +-
builtin/describe.c | 2 +-
builtin/difftool.c | 6 +++---
builtin/fetch.c | 2 +-
config.c | 2 +-
diff.c | 2 +-
diffcore-rename.c | 2 +-
hashmap.c | 6 +++---
hashmap.h | 4 ++--
merge-recursive.c | 4 ++--
name-hash.c | 8 ++++----
packfile.c | 2 +-
patch-ids.c | 2 +-
range-diff.c | 2 +-
ref-filter.c | 2 +-
sequencer.c | 2 +-
sub-process.c | 2 +-
submodule-config.c | 2 +-
t/helper/test-hashmap.c | 6 +++---
20 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/attr.c b/attr.c
index a8be7a7b4f..fa26a3e3cb 100644
--- a/attr.c
+++ b/attr.c
@@ -122,7 +122,7 @@ static void attr_hashmap_add(struct attr_hashmap *map,
e->keylen = keylen;
e->value = value;
- hashmap_add(&map->map, e);
+ hashmap_add(&map->map, &e->ent);
}
struct all_attrs_item {
diff --git a/blame.c b/blame.c
index 46059410cd..4d20aee435 100644
--- a/blame.c
+++ b/blame.c
@@ -424,7 +424,7 @@ static void get_fingerprint(struct fingerprint *result,
found_entry->count += 1;
} else {
entry->count = 1;
- hashmap_add(&result->map, entry);
+ hashmap_add(&result->map, &entry->entry);
++entry;
}
}
diff --git a/builtin/describe.c b/builtin/describe.c
index 596ddf89a5..f5e0a7e033 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -124,7 +124,7 @@ static void add_to_known_names(const char *path,
e = xmalloc(sizeof(struct commit_name));
oidcpy(&e->peeled, peeled);
hashmap_entry_init(&e->entry, oidhash(peeled));
- hashmap_add(&names, e);
+ hashmap_add(&names, &e->entry);
e->path = NULL;
}
e->tag = tag;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 98ffc04c61..82c146718d 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -168,7 +168,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
e = existing;
} else {
e->left[0] = e->right[0] = '\0';
- hashmap_add(map, e);
+ hashmap_add(map, &e->entry);
}
strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
}
@@ -235,7 +235,7 @@ static void changed_files(struct hashmap *result, const char *index_path,
struct path_entry *entry;
FLEX_ALLOC_STR(entry, path, buf.buf);
hashmap_entry_init(&entry->entry, strhash(buf.buf));
- hashmap_add(result, entry);
+ hashmap_add(result, &entry->entry);
}
fclose(fp);
if (finish_command(&diff_files))
@@ -466,7 +466,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
free(entry);
continue;
}
- hashmap_add(&working_tree_dups, entry);
+ hashmap_add(&working_tree_dups, &entry->entry);
if (!use_wt_file(workdir, dst_path, &roid)) {
if (checkout_path(rmode, &roid, dst_path,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b7d70eee70..909dbde909 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -278,7 +278,7 @@ static struct refname_hash_entry *refname_hash_add(struct hashmap *map,
FLEX_ALLOC_MEM(ent, refname, refname, len);
hashmap_entry_init(&ent->ent, strhash(refname));
oidcpy(&ent->oid, oid);
- hashmap_add(map, ent);
+ hashmap_add(map, &ent->ent);
return ent;
}
diff --git a/config.c b/config.c
index 08d866e7de..2243d7c3d6 100644
--- a/config.c
+++ b/config.c
@@ -1885,7 +1885,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
hashmap_entry_init(&e->ent, strhash(key));
e->key = xstrdup(key);
string_list_init(&e->value_list, 1);
- hashmap_add(&cs->config_hash, e);
+ hashmap_add(&cs->config_hash, &e->ent);
}
si = string_list_append_nodup(&e->value_list, xstrdup_or_null(value));
diff --git a/diff.c b/diff.c
index 1168f0cbb9..cc7f06d10d 100644
--- a/diff.c
+++ b/diff.c
@@ -1003,7 +1003,7 @@ static void add_lines_to_move_detection(struct diff_options *o,
if (prev_line && prev_line->es->s == o->emitted_symbols->buf[n].s)
prev_line->next_line = key;
- hashmap_add(hm, key);
+ hashmap_add(hm, &key->ent);
prev_line = key;
}
}
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 2a1449013b..4670a40179 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -330,7 +330,7 @@ static void insert_file_table(struct repository *r,
entry->filespec = filespec;
hashmap_entry_init(&entry->entry, hash_filespec(r, filespec));
- hashmap_add(table, entry);
+ hashmap_add(table, &entry->entry);
}
/*
diff --git a/hashmap.c b/hashmap.c
index c1de40eea0..9c2db3e0c8 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -201,12 +201,12 @@ void *hashmap_get_next(const struct hashmap *map,
return NULL;
}
-void hashmap_add(struct hashmap *map, void *entry)
+void hashmap_add(struct hashmap *map, struct hashmap_entry *entry)
{
unsigned int b = bucket(map, entry);
/* add entry */
- ((struct hashmap_entry *) entry)->next = map->table[b];
+ entry->next = map->table[b];
map->table[b] = entry;
/* fix size and rehash if appropriate */
@@ -302,7 +302,7 @@ const void *memintern(const void *data, size_t len)
FLEX_ALLOC_MEM(e, data, data, len);
hashmap_entry_init(&e->ent, key.ent.hash);
e->len = len;
- hashmap_add(&map, e);
+ hashmap_add(&map, &e->ent);
}
return e->data;
}
diff --git a/hashmap.h b/hashmap.h
index 93fb9599ca..47ee5c00d7 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -50,7 +50,7 @@
* FLEX_ALLOC_STR(e, value, value);
* hashmap_entry_init(&e->ent, memhash(&key, sizeof(long)));
* e->key = key;
- * hashmap_add(&map, e);
+ * hashmap_add(&map, &e->ent);
* }
*
* if (!strcmp("print_all_by_key", action)) {
@@ -328,7 +328,7 @@ void *hashmap_get_next(const struct hashmap *map,
* `map` is the hashmap structure.
* `entry` is the entry to add.
*/
-void hashmap_add(struct hashmap *map, void *entry);
+void hashmap_add(struct hashmap *map, struct hashmap_entry *entry);
/*
* Adds or replaces a hashmap entry. If the hashmap contains duplicate
diff --git a/merge-recursive.c b/merge-recursive.c
index 6bc4f14ff4..db9b247ece 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -455,7 +455,7 @@ static int save_files_dirs(const struct object_id *oid,
FLEX_ALLOC_MEM(entry, path, base->buf, base->len);
hashmap_entry_init(&entry->e, path_hash(entry->path));
- hashmap_add(&opt->current_file_dir_set, entry);
+ hashmap_add(&opt->current_file_dir_set, &entry->e);
strbuf_setlen(base, baselen);
return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
@@ -732,7 +732,7 @@ static char *unique_path(struct merge_options *opt, const char *path, const char
FLEX_ALLOC_MEM(entry, path, newpath.buf, newpath.len);
hashmap_entry_init(&entry->e, path_hash(entry->path));
- hashmap_add(&opt->current_file_dir_set, entry);
+ hashmap_add(&opt->current_file_dir_set, &entry->e);
return strbuf_detach(&newpath, NULL);
}
diff --git a/name-hash.c b/name-hash.c
index 4d84326c58..faec682bc7 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -70,7 +70,7 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,
FLEX_ALLOC_MEM(dir, name, ce->name, namelen);
hashmap_entry_init(&dir->ent, memihash(ce->name, namelen));
dir->namelen = namelen;
- hashmap_add(&istate->dir_hash, dir);
+ hashmap_add(&istate->dir_hash, &dir->ent);
/* recursively add missing parent directories */
dir->parent = hash_dir_entry(istate, ce, namelen);
@@ -107,7 +107,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
return;
ce->ce_flags |= CE_HASHED;
hashmap_entry_init(&ce->ent, memihash(ce->name, ce_namelen(ce)));
- hashmap_add(&istate->name_hash, ce);
+ hashmap_add(&istate->name_hash, &ce->ent);
if (ignore_case)
add_dir_entry(istate, ce);
@@ -283,7 +283,7 @@ static struct dir_entry *hash_dir_entry_with_parent_and_prefix(
hashmap_entry_init(&dir->ent, hash);
dir->namelen = prefix->len;
dir->parent = parent;
- hashmap_add(&istate->dir_hash, dir);
+ hashmap_add(&istate->dir_hash, &dir->ent);
if (parent) {
unlock_dir_mutex(lock_nr);
@@ -473,7 +473,7 @@ static void *lazy_name_thread_proc(void *_data)
struct cache_entry *ce_k = d->istate->cache[k];
ce_k->ce_flags |= CE_HASHED;
hashmap_entry_init(&ce_k->ent, d->lazy_entries[k].hash_name);
- hashmap_add(&d->istate->name_hash, ce_k);
+ hashmap_add(&d->istate->name_hash, &ce_k->ent);
}
return NULL;
diff --git a/packfile.c b/packfile.c
index 96535eb86b..f7402c470b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1488,7 +1488,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
if (!delta_base_cache.cmpfn)
hashmap_init(&delta_base_cache, delta_base_cache_hash_cmp, NULL, 0);
hashmap_entry_init(&ent->ent, pack_entry_hash(p, base_offset));
- hashmap_add(&delta_base_cache, ent);
+ hashmap_add(&delta_base_cache, &ent->ent);
}
int packed_object_info(struct repository *r, struct packed_git *p,
diff --git a/patch-ids.c b/patch-ids.c
index a2da711678..f87b62bf58 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -116,6 +116,6 @@ struct patch_id *add_commit_patch_id(struct commit *commit,
return NULL;
}
- hashmap_add(&ids->patches, key);
+ hashmap_add(&ids->patches, &key->ent);
return key;
}
diff --git a/range-diff.c b/range-diff.c
index 32b29f9594..96f955d84d 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -218,7 +218,7 @@ static void find_exact_matches(struct string_list *a, struct string_list *b)
util->patch = a->items[i].string;
util->diff = util->patch + util->diff_offset;
hashmap_entry_init(&util->e, strhash(util->diff));
- hashmap_add(&map, util);
+ hashmap_add(&map, &util->e);
}
/* Now try to find exact matches in b */
diff --git a/ref-filter.c b/ref-filter.c
index 206014c93d..d939ebc6bb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1568,7 +1568,7 @@ static void populate_worktree_map(struct hashmap *map, struct worktree **worktre
hashmap_entry_init(&entry->ent,
strhash(worktrees[i]->head_ref));
- hashmap_add(map, entry);
+ hashmap_add(map, &entry->ent);
}
}
}
diff --git a/sequencer.c b/sequencer.c
index 1140cdf526..ee11cda7e7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4539,7 +4539,7 @@ static const char *label_oid(struct object_id *oid, const char *label,
FLEX_ALLOC_STR(labels_entry, label, label);
hashmap_entry_init(&labels_entry->entry, strihash(label));
- hashmap_add(&state->labels, labels_entry);
+ hashmap_add(&state->labels, &labels_entry->entry);
FLEX_ALLOC_STR(string_entry, string, label);
oidcpy(&string_entry->entry.oid, oid);
diff --git a/sub-process.c b/sub-process.c
index 9847dad6fc..d58e069855 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -105,7 +105,7 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
return err;
}
- hashmap_add(hashmap, entry);
+ hashmap_add(hashmap, &entry->ent);
return 0;
}
diff --git a/submodule-config.c b/submodule-config.c
index 4aa02e280e..a3bbd9fd6f 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -149,7 +149,7 @@ static void cache_add(struct submodule_cache *cache,
struct submodule_entry *e = xmalloc(sizeof(*e));
hashmap_entry_init(&e->ent, hash);
e->config = submodule;
- hashmap_add(&cache->for_name, e);
+ hashmap_add(&cache->for_name, &e->ent);
}
static const struct submodule *cache_lookup_path(struct submodule_cache *cache,
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index bf063a2521..49e715f1cd 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -104,7 +104,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
/* add entries */
for (i = 0; i < TEST_SIZE; i++) {
hashmap_entry_init(&entries[i]->ent, hashes[i]);
- hashmap_add(&map, entries[i]);
+ hashmap_add(&map, &entries[i]->ent);
}
hashmap_free(&map, 0);
@@ -117,7 +117,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
j = (method & TEST_SPARSE) ? TEST_SIZE / 10 : TEST_SIZE;
for (i = 0; i < j; i++) {
hashmap_entry_init(&entries[i]->ent, hashes[i]);
- hashmap_add(&map, entries[i]);
+ hashmap_add(&map, &entries[i]->ent);
}
for (j = 0; j < rounds; j++) {
@@ -179,7 +179,7 @@ int cmd__hashmap(int argc, const char **argv)
entry = alloc_test_entry(hash, p1, p2);
/* add to hashmap */
- hashmap_add(&map, entry);
+ hashmap_add(&map, &entry->ent);
} else if (!strcmp("put", cmd) && p1 && p2) {
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 07/20] hashmap_get takes "const struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (5 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 06/20] hashmap_add takes "struct " Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 08/20] hashmap_remove " Eric Wong
` (15 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
This is less error-prone than "const void *" as the compiler
now detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
attr.c | 2 +-
blame.c | 6 +++---
builtin/difftool.c | 5 +++--
builtin/fast-export.c | 2 +-
config.c | 2 +-
diff.c | 4 ++--
hashmap.c | 5 +++--
hashmap.h | 8 +++++---
merge-recursive.c | 4 ++--
name-hash.c | 2 +-
patch-ids.c | 2 +-
revision.c | 3 ++-
sub-process.c | 2 +-
submodule-config.c | 4 ++--
14 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/attr.c b/attr.c
index fa26a3e3cb..9bdef61cc3 100644
--- a/attr.c
+++ b/attr.c
@@ -101,7 +101,7 @@ static void *attr_hashmap_get(struct attr_hashmap *map,
hashmap_entry_init(&k.ent, memhash(key, keylen));
k.key = key;
k.keylen = keylen;
- e = hashmap_get(&map->map, &k, NULL);
+ e = hashmap_get(&map->map, &k.ent, NULL);
return e ? e->value : NULL;
}
diff --git a/blame.c b/blame.c
index 4d20aee435..73ffb59403 100644
--- a/blame.c
+++ b/blame.c
@@ -419,7 +419,7 @@ static void get_fingerprint(struct fingerprint *result,
continue;
hashmap_entry_init(&entry->entry, hash);
- found_entry = hashmap_get(&result->map, entry, NULL);
+ found_entry = hashmap_get(&result->map, &entry->entry, NULL);
if (found_entry) {
found_entry->count += 1;
} else {
@@ -452,7 +452,7 @@ static int fingerprint_similarity(struct fingerprint *a, struct fingerprint *b)
hashmap_iter_init(&b->map, &iter);
while ((entry_b = hashmap_iter_next(&iter))) {
- if ((entry_a = hashmap_get(&a->map, entry_b, NULL))) {
+ if ((entry_a = hashmap_get(&a->map, &entry_b->entry, NULL))) {
intersection += entry_a->count < entry_b->count ?
entry_a->count : entry_b->count;
}
@@ -471,7 +471,7 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b)
hashmap_iter_init(&b->map, &iter);
while ((entry_b = hashmap_iter_next(&iter))) {
- if ((entry_a = hashmap_get(&a->map, entry_b, NULL))) {
+ if ((entry_a = hashmap_get(&a->map, &entry_b->entry, NULL))) {
if (entry_a->count <= entry_b->count)
hashmap_remove(&a->map, entry_b, NULL);
else
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 82c146718d..f41298d199 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -162,7 +162,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
FLEX_ALLOC_STR(e, path, path);
hashmap_entry_init(&e->entry, strhash(path));
- existing = hashmap_get(map, e, NULL);
+ existing = hashmap_get(map, &e->entry, NULL);
if (existing) {
free(e);
e = existing;
@@ -462,7 +462,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
/* Avoid duplicate working_tree entries */
FLEX_ALLOC_STR(entry, path, dst_path);
hashmap_entry_init(&entry->entry, strhash(dst_path));
- if (hashmap_get(&working_tree_dups, entry, NULL)) {
+ if (hashmap_get(&working_tree_dups, &entry->entry,
+ NULL)) {
free(entry);
continue;
}
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 287dbd24a3..c693cf6a8c 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -151,7 +151,7 @@ static const void *anonymize_mem(struct hashmap *map,
hashmap_entry_init(&key.hash, memhash(orig, *len));
key.orig = orig;
key.orig_len = *len;
- ret = hashmap_get(map, &key, NULL);
+ ret = hashmap_get(map, &key.hash, NULL);
if (!ret) {
ret = xmalloc(sizeof(*ret));
diff --git a/config.c b/config.c
index 2243d7c3d6..1a1b6675fd 100644
--- a/config.c
+++ b/config.c
@@ -1863,7 +1863,7 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
hashmap_entry_init(&k.ent, strhash(normalized_key));
k.key = normalized_key;
- found_entry = hashmap_get(&cs->config_hash, &k, NULL);
+ found_entry = hashmap_get(&cs->config_hash, &k.ent, NULL);
free(normalized_key);
return found_entry;
}
diff --git a/diff.c b/diff.c
index cc7f06d10d..72d3c6aa19 100644
--- a/diff.c
+++ b/diff.c
@@ -1144,13 +1144,13 @@ 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, NULL);
+ match = 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, NULL);
+ match = hashmap_get(hm, &key->ent, NULL);
free(key);
break;
default:
diff --git a/hashmap.c b/hashmap.c
index 9c2db3e0c8..092236c09a 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -186,7 +186,8 @@ void hashmap_free(struct hashmap *map, int free_entries)
memset(map, 0, sizeof(*map));
}
-void *hashmap_get(const struct hashmap *map, const void *key, const void *keydata)
+void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
+ const void *keydata)
{
return *find_entry_ptr(map, key, keydata);
}
@@ -296,7 +297,7 @@ const void *memintern(const void *data, size_t len)
/* lookup interned string in pool */
hashmap_entry_init(&key.ent, memhash(data, len));
key.len = len;
- e = hashmap_get(&map, &key, data);
+ e = hashmap_get(&map, &key.ent, data);
if (!e) {
/* not found: create it */
FLEX_ALLOC_MEM(e, data, data, len);
diff --git a/hashmap.h b/hashmap.h
index 47ee5c00d7..ef83f9431d 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -74,7 +74,8 @@
* e->key = key;
*
* flags |= COMPARE_VALUE;
- * printf("%sfound\n", hashmap_get(&map, e, NULL) ? "" : "not ");
+ * printf("%sfound\n",
+ * hashmap_get(&map, &e->ent, NULL) ? "" : "not ");
* free(e);
* }
*
@@ -84,7 +85,8 @@
* k.key = key;
*
* flags |= COMPARE_VALUE;
- * printf("%sfound\n", hashmap_get(&map, &k, value) ? "" : "not ");
+ * printf("%sfound\n",
+ * hashmap_get(&map, &k->ent, value) ? "" : "not ");
* }
*
* if (!strcmp("end", action)) {
@@ -286,7 +288,7 @@ static inline unsigned int hashmap_get_size(struct hashmap *map)
* If an entry with matching hash code is found, `key` and `keydata` are passed
* to `hashmap_cmp_fn` to decide whether the entry matches the key.
*/
-void *hashmap_get(const struct hashmap *map, const void *key,
+void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
const void *keydata);
/*
diff --git a/merge-recursive.c b/merge-recursive.c
index db9b247ece..2d31a3e279 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -63,7 +63,7 @@ static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
return NULL;
hashmap_entry_init(&key.ent, strhash(dir));
key.dir = dir;
- return hashmap_get(hashmap, &key, NULL);
+ return hashmap_get(hashmap, &key.ent, NULL);
}
static int dir_rename_cmp(const void *unused_cmp_data,
@@ -99,7 +99,7 @@ static struct collision_entry *collision_find_entry(struct hashmap *hashmap,
hashmap_entry_init(&key.ent, strhash(target_file));
key.target_file = target_file;
- return hashmap_get(hashmap, &key, NULL);
+ return hashmap_get(hashmap, &key.ent, NULL);
}
static int collision_cmp(void *unused_cmp_data,
diff --git a/name-hash.c b/name-hash.c
index faec682bc7..4eaeded775 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -35,7 +35,7 @@ static struct dir_entry *find_dir_entry__hash(struct index_state *istate,
struct dir_entry key;
hashmap_entry_init(&key.ent, hash);
key.namelen = namelen;
- return hashmap_get(&istate->dir_hash, &key, name);
+ return hashmap_get(&istate->dir_hash, &key.ent, name);
}
static struct dir_entry *find_dir_entry(struct index_state *istate,
diff --git a/patch-ids.c b/patch-ids.c
index f87b62bf58..437f29e42c 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -99,7 +99,7 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
if (init_patch_id_entry(&patch, commit, ids))
return NULL;
- return hashmap_get(&ids->patches, &patch, NULL);
+ return hashmap_get(&ids->patches, &patch.ent, NULL);
}
struct patch_id *add_commit_patch_id(struct commit *commit,
diff --git a/revision.c b/revision.c
index 3461c78883..4336281286 100644
--- a/revision.c
+++ b/revision.c
@@ -147,7 +147,8 @@ static void paths_and_oids_insert(struct hashmap *map,
key.path = (char *)path;
oidset_init(&key.trees, 0);
- if (!(entry = (struct path_and_oids_entry *)hashmap_get(map, &key, NULL))) {
+ entry = hashmap_get(map, &key.ent, NULL);
+ if (!entry) {
entry = xcalloc(1, sizeof(struct path_and_oids_entry));
hashmap_entry_init(&entry->ent, hash);
entry->path = xstrdup(key.path);
diff --git a/sub-process.c b/sub-process.c
index d58e069855..debd86bb68 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -22,7 +22,7 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch
hashmap_entry_init(&key.ent, strhash(cmd));
key.cmd = cmd;
- return hashmap_get(hashmap, &key, NULL);
+ return hashmap_get(hashmap, &key.ent, NULL);
}
int subprocess_read_status(int fd, struct strbuf *status)
diff --git a/submodule-config.c b/submodule-config.c
index a3bbd9fd6f..58d585cd7d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -166,7 +166,7 @@ static const struct submodule *cache_lookup_path(struct submodule_cache *cache,
hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
- entry = hashmap_get(&cache->for_path, &key, NULL);
+ entry = hashmap_get(&cache->for_path, &key.ent, NULL);
if (entry)
return entry->config;
return NULL;
@@ -186,7 +186,7 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
- entry = hashmap_get(&cache->for_name, &key, NULL);
+ entry = hashmap_get(&cache->for_name, &key.ent, NULL);
if (entry)
return entry->config;
return NULL;
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 08/20] hashmap_remove takes "const struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (6 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 07/20] hashmap_get takes "const struct " Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 09/20] hashmap_put takes "struct " Eric Wong
` (14 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
This is less error-prone than "const void *" as the compiler
now detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
blame.c | 2 +-
hashmap.c | 3 ++-
hashmap.h | 2 +-
merge-recursive.c | 2 +-
name-hash.c | 4 ++--
packfile.c | 2 +-
range-diff.c | 2 +-
sub-process.c | 2 +-
submodule-config.c | 2 +-
9 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/blame.c b/blame.c
index 73ffb59403..00f8f3fb0a 100644
--- a/blame.c
+++ b/blame.c
@@ -473,7 +473,7 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b)
while ((entry_b = hashmap_iter_next(&iter))) {
if ((entry_a = hashmap_get(&a->map, &entry_b->entry, NULL))) {
if (entry_a->count <= entry_b->count)
- hashmap_remove(&a->map, entry_b, NULL);
+ hashmap_remove(&a->map, &entry_b->entry, NULL);
else
entry_a->count -= entry_b->count;
}
diff --git a/hashmap.c b/hashmap.c
index 092236c09a..bdf33e0381 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -218,7 +218,8 @@ void hashmap_add(struct hashmap *map, struct hashmap_entry *entry)
}
}
-void *hashmap_remove(struct hashmap *map, const void *key, const void *keydata)
+void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
+ const void *keydata)
{
struct hashmap_entry *old;
struct hashmap_entry **e = find_entry_ptr(map, key, keydata);
diff --git a/hashmap.h b/hashmap.h
index ef83f9431d..c4c31b462f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -349,7 +349,7 @@ void *hashmap_put(struct hashmap *map, void *entry);
*
* Argument explanation is the same as in `hashmap_get`.
*/
-void *hashmap_remove(struct hashmap *map, const void *key,
+void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
const void *keydata);
/*
diff --git a/merge-recursive.c b/merge-recursive.c
index 2d31a3e279..f60451d396 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2001,7 +2001,7 @@ static void remove_hashmap_entries(struct hashmap *dir_renames,
for (i = 0; i < items_to_remove->nr; i++) {
entry = items_to_remove->items[i].util;
- hashmap_remove(dir_renames, entry, NULL);
+ hashmap_remove(dir_renames, &entry->ent, NULL);
}
string_list_clear(items_to_remove, 0);
}
diff --git a/name-hash.c b/name-hash.c
index 4eaeded775..44d788f1ce 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -95,7 +95,7 @@ static void remove_dir_entry(struct index_state *istate, struct cache_entry *ce)
struct dir_entry *dir = hash_dir_entry(istate, ce, ce_namelen(ce));
while (dir && !(--dir->nr)) {
struct dir_entry *parent = dir->parent;
- hashmap_remove(&istate->dir_hash, dir, NULL);
+ hashmap_remove(&istate->dir_hash, &dir->ent, NULL);
free(dir);
dir = parent;
}
@@ -625,7 +625,7 @@ void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
if (!istate->name_hash_initialized || !(ce->ce_flags & CE_HASHED))
return;
ce->ce_flags &= ~CE_HASHED;
- hashmap_remove(&istate->name_hash, ce, ce);
+ hashmap_remove(&istate->name_hash, &ce->ent, ce);
if (ignore_case)
remove_dir_entry(istate, ce);
diff --git a/packfile.c b/packfile.c
index f7402c470b..3edd648de0 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1423,7 +1423,7 @@ static int in_delta_base_cache(struct packed_git *p, off_t base_offset)
*/
static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)
{
- hashmap_remove(&delta_base_cache, ent, &ent->key);
+ hashmap_remove(&delta_base_cache, &ent->ent, &ent->key);
list_del(&ent->lru);
delta_base_cached -= ent->size;
free(ent);
diff --git a/range-diff.c b/range-diff.c
index 96f955d84d..c51cfd5556 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -229,7 +229,7 @@ static void find_exact_matches(struct string_list *a, struct string_list *b)
util->patch = b->items[i].string;
util->diff = util->patch + util->diff_offset;
hashmap_entry_init(&util->e, strhash(util->diff));
- other = hashmap_remove(&map, util, NULL);
+ other = hashmap_remove(&map, &util->e, NULL);
if (other) {
if (other->matching >= 0)
BUG("already assigned!");
diff --git a/sub-process.c b/sub-process.c
index debd86bb68..99fccef592 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -58,7 +58,7 @@ void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry)
kill(entry->process.pid, SIGTERM);
finish_command(&entry->process);
- hashmap_remove(hashmap, entry, NULL);
+ hashmap_remove(hashmap, &entry->ent, NULL);
}
static void subprocess_exit_handler(struct child_process *process)
diff --git a/submodule-config.c b/submodule-config.c
index 58d585cd7d..7486745a6a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -137,7 +137,7 @@ static void cache_remove_path(struct submodule_cache *cache,
struct submodule_entry *removed;
hashmap_entry_init(&e.ent, hash);
e.config = submodule;
- removed = hashmap_remove(&cache->for_path, &e, NULL);
+ removed = hashmap_remove(&cache->for_path, &e.ent, NULL);
free(removed);
}
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 09/20] hashmap_put takes "struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (7 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 08/20] hashmap_remove " Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 10/20] introduce container_of macro Eric Wong
` (13 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
This is less error-prone than "void *" as the compiler now
detects invalid types being passed.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
builtin/fast-export.c | 2 +-
hashmap.c | 2 +-
hashmap.h | 2 +-
merge-recursive.c | 4 ++--
oidmap.c | 2 +-
refs.c | 5 ++++-
remote.c | 2 +-
revision.c | 2 +-
sequencer.c | 2 +-
submodule-config.c | 2 +-
t/helper/test-hashmap.c | 2 +-
11 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index c693cf6a8c..192e21dae4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -160,7 +160,7 @@ static const void *anonymize_mem(struct hashmap *map,
ret->orig_len = *len;
ret->anon = generate(orig, len);
ret->anon_len = *len;
- hashmap_put(map, ret);
+ hashmap_put(map, &ret->hash);
}
*len = ret->anon_len;
diff --git a/hashmap.c b/hashmap.c
index bdf33e0381..9b83e73d03 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -241,7 +241,7 @@ void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
return old;
}
-void *hashmap_put(struct hashmap *map, void *entry)
+void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry)
{
struct hashmap_entry *old = hashmap_remove(map, entry, NULL);
hashmap_add(map, entry);
diff --git a/hashmap.h b/hashmap.h
index c4c31b462f..d122c75d0f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -340,7 +340,7 @@ void hashmap_add(struct hashmap *map, struct hashmap_entry *entry);
* `entry` is the entry to add or replace.
* Returns the replaced entry, or NULL if not found (i.e. the entry was added).
*/
-void *hashmap_put(struct hashmap *map, void *entry);
+void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
/*
* Removes a hashmap entry matching the specified key. If the hashmap contains
diff --git a/merge-recursive.c b/merge-recursive.c
index f60451d396..a685b4fb69 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2229,7 +2229,7 @@ static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)
if (!entry) {
entry = xmalloc(sizeof(*entry));
dir_rename_entry_init(entry, old_dir);
- hashmap_put(dir_renames, entry);
+ hashmap_put(dir_renames, &entry->ent);
} else {
free(old_dir);
}
@@ -2360,7 +2360,7 @@ static void compute_collisions(struct hashmap *collisions,
sizeof(struct collision_entry));
hashmap_entry_init(&collision_ent->ent,
strhash(new_path));
- hashmap_put(collisions, collision_ent);
+ hashmap_put(collisions, &collision_ent->ent);
collision_ent->target_file = new_path;
} else {
free(new_path);
diff --git a/oidmap.c b/oidmap.c
index 6d6e840d03..cd22b3a8bf 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -51,5 +51,5 @@ void *oidmap_put(struct oidmap *map, void *entry)
oidmap_init(map, 0);
hashmap_entry_init(&to_put->internal_entry, oidhash(&to_put->oid));
- return hashmap_put(&map->map, to_put);
+ return hashmap_put(&map->map, &to_put->internal_entry);
}
diff --git a/refs.c b/refs.c
index c43ec59c6e..3e55031256 100644
--- a/refs.c
+++ b/refs.c
@@ -1863,10 +1863,13 @@ static void register_ref_store_map(struct hashmap *map,
struct ref_store *refs,
const char *name)
{
+ struct ref_store_hash_entry *entry;
+
if (!map->tablesize)
hashmap_init(map, ref_store_hash_cmp, NULL, 0);
- if (hashmap_put(map, alloc_ref_store_hash_entry(name, refs)))
+ entry = alloc_ref_store_hash_entry(name, refs);
+ if (hashmap_put(map, &entry->ent))
BUG("%s ref_store '%s' initialized twice", type, name);
}
diff --git a/remote.c b/remote.c
index bd81cb71bc..8ca23d95dc 100644
--- a/remote.c
+++ b/remote.c
@@ -159,7 +159,7 @@ static struct remote *make_remote(const char *name, int len)
remotes[remotes_nr++] = ret;
hashmap_entry_init(&ret->ent, lookup_entry.hash);
- replaced = hashmap_put(&remotes_hash, ret);
+ replaced = hashmap_put(&remotes_hash, &ret->ent);
assert(replaced == NULL); /* no previous entry overwritten */
return ret;
}
diff --git a/revision.c b/revision.c
index 4336281286..a7e2339064 100644
--- a/revision.c
+++ b/revision.c
@@ -153,7 +153,7 @@ static void paths_and_oids_insert(struct hashmap *map,
hashmap_entry_init(&entry->ent, hash);
entry->path = xstrdup(key.path);
oidset_init(&entry->trees, 16);
- hashmap_put(map, entry);
+ hashmap_put(map, &entry->ent);
}
oidset_insert(&entry->trees, oid);
diff --git a/sequencer.c b/sequencer.c
index ee11cda7e7..b4ef70e260 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5254,7 +5254,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
entry->i = i;
hashmap_entry_init(&entry->entry,
strhash(entry->subject));
- hashmap_put(&subject2item, entry);
+ hashmap_put(&subject2item, &entry->entry);
}
}
diff --git a/submodule-config.c b/submodule-config.c
index 7486745a6a..9248c5ea5b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -125,7 +125,7 @@ static void cache_put_path(struct submodule_cache *cache,
struct submodule_entry *e = xmalloc(sizeof(*e));
hashmap_entry_init(&e->ent, hash);
e->config = submodule;
- hashmap_put(&cache->for_path, e);
+ hashmap_put(&cache->for_path, &e->ent);
}
static void cache_remove_path(struct submodule_cache *cache,
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 49e715f1cd..de2bd083b9 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -187,7 +187,7 @@ int cmd__hashmap(int argc, const char **argv)
entry = alloc_test_entry(hash, p1, p2);
/* add / replace entry */
- entry = hashmap_put(&map, entry);
+ entry = hashmap_put(&map, &entry->ent);
/* print and free replaced entry, if any */
puts(entry ? get_value(entry) : "NULL");
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 10/20] introduce container_of macro
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (8 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 09/20] hashmap_put takes "struct " Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 11/20] hashmap_get_next returns "struct hashmap_entry *" Eric Wong
` (12 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
This macro is popular within the Linux kernel for supporting
intrusive data structures such as linked lists, red-black trees,
and chained hash tables while allowing the compiler to do
type checking.
Later patches will use container_of() to remove the limitation
of "hashmap_entry" being location-dependent. This will complete
the transition to compile-time type checking for the hashmap API.
This macro already exists in our source as "list_entry" in
list.h and making "list_entry" an alias to "container_of"
as the Linux kernel has done is a possibility.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
git-compat-util.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 83be89de0a..4cc2c8283a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1312,4 +1312,14 @@ void unleak_memory(const void *ptr, size_t len);
*/
#include "banned.h"
+/*
+ * container_of - Get the address of an object containing a field.
+ *
+ * @ptr: pointer to the field.
+ * @type: type of the object.
+ * @member: name of the field within the object.
+ */
+#define container_of(ptr, type, member) \
+ ((type *) ((char *)(ptr) - offsetof(type, member)))
+
#endif
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 11/20] hashmap_get_next returns "struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (9 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 10/20] introduce container_of macro Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 12/20] hashmap: use *_entry APIs to wrap container_of Eric Wong
` (11 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
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>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
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..0978814086 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;
- for (; match; match = hashmap_get_next(hm, &match->ent)) {
+ for (ent = &match->ent; 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 9b83e73d03..22bc7c5b3b 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 d122c75d0f..21bc8aff2b 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 44d788f1ce..dbacb34f50 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -702,15 +702,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) {
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 12/20] hashmap: use *_entry APIs to wrap container_of
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (10 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 11/20] hashmap_get_next returns "struct hashmap_entry *" Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 13/20] hashmap_get{,_from_hash} return "struct hashmap_entry *" Eric Wong
` (10 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Using `container_of' can be verbose and choosing names for
intermediate "struct hashmap_entry" pointers is a hard problem.
So introduce "*_entry" APIs inspired by similar linked-list
APIs in the Linux kernel.
Unfortunately, `__typeof__' is not portable C, so we need an
extra parameter to specify the type.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
diff.c | 21 +++++++++------------
diffcore-rename.c | 14 +++++---------
git-compat-util.h | 15 +++++++++++++++
hashmap.h | 40 ++++++++++++++++++++++++++++++++++------
name-hash.c | 11 +++++------
t/helper/test-hashmap.c | 12 +++++-------
6 files changed, 73 insertions(+), 40 deletions(-)
diff --git a/diff.c b/diff.c
index 0978814086..66cdf4e9ca 100644
--- a/diff.c
+++ b/diff.c
@@ -1035,10 +1035,8 @@ 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;
- for (ent = &match->ent; ent; ent = hashmap_get_next(hm, ent)) {
- match = container_of(ent, struct moved_entry, ent);
+ hashmap_for_each_entry_from(hm, match, 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) ?
@@ -1137,9 +1135,8 @@ 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;
+ struct moved_entry *match = NULL;
struct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];
enum diff_symbol last_symbol = 0;
@@ -1147,20 +1144,22 @@ static void mark_color_as_moved(struct diff_options *o,
case DIFF_SYMBOL_PLUS:
hm = del_lines;
key = prepare_entry(o, n);
- ent = hashmap_get(hm, &key->ent, NULL);
+ match = hashmap_get_entry(hm, key, NULL,
+ struct moved_entry, ent);
free(key);
break;
case DIFF_SYMBOL_MINUS:
hm = add_lines;
key = prepare_entry(o, n);
- ent = hashmap_get(hm, &key->ent, NULL);
+ match = hashmap_get_entry(hm, key, NULL,
+ struct moved_entry, ent);
free(key);
break;
default:
flipped_block = 0;
}
- if (!ent) {
+ if (!match) {
int i;
adjust_last_block(o, n, block_length);
@@ -1172,7 +1171,6 @@ 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;
@@ -1193,9 +1191,8 @@ 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 (; ent; ent = hashmap_get_next(hm, ent)) {
- match = container_of(ent, struct moved_entry,
- ent);
+ hashmap_for_each_entry_from(hm, match,
+ 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 71aa240a68..611b08f463 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -274,23 +274,19 @@ 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;
+ unsigned int hash = hash_filespec(options->repo, target);
/*
* Find the best source match for specified destination.
*/
- ent = hashmap_get_from_hash(srcs,
- hash_filespec(options->repo, target),
- NULL);
- for (; ent; ent = hashmap_get_next(srcs, ent)) {
+ p = hashmap_get_entry_from_hash(srcs, hash, NULL,
+ struct file_similarity, entry);
+ hashmap_for_each_entry_from(srcs, p, struct file_similarity, entry) {
int score;
- struct diff_filespec *source;
-
- p = container_of(ent, struct file_similarity, entry);
- source = p->filespec;
+ struct diff_filespec *source = p->filespec;
/* False hash collision? */
if (!oideq(&source->oid, &target->oid))
diff --git a/git-compat-util.h b/git-compat-util.h
index 4cc2c8283a..4a23b9090b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1322,4 +1322,19 @@ void unleak_memory(const void *ptr, size_t len);
#define container_of(ptr, type, member) \
((type *) ((char *)(ptr) - offsetof(type, member)))
+/*
+ * helper function for `container_of_or_null' to avoid multiple
+ * evaluation of @ptr
+ */
+static inline void *container_of_or_null_offset(void *ptr, size_t offset)
+{
+ return ptr ? (char *)ptr - offset : NULL;
+}
+
+/*
+ * like `container_of', but allows returned value to be NULL
+ */
+#define container_of_or_null(ptr, type, member) \
+ (type *)container_of_or_null_offset(ptr, offsetof(type, member))
+
#endif
diff --git a/hashmap.h b/hashmap.h
index 21bc8aff2b..cd42dcc15c 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -55,17 +55,15 @@
*
* 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;
- * ent = hashmap_get(&map, &k, NULL);
- * if (ent) {
- * e = container_of(ent, struct long2string, ent);
+ * e = hashmap_get_entry(&map, &k, NULL, struct long2string, ent);
+ * if (e) {
* printf("first: %ld %s\n", e->key, e->value);
- * while ((ent = hashmap_get_next(&map, ent))) {
- * e = container_of(ent, struct long2string, ent);
+ * while ((e = hashmap_get_next_entry(&map, e,
+ * struct long2string, ent))) {
* printf("found more: %ld %s\n", e->key, e->value);
* }
* }
@@ -387,6 +385,36 @@ static inline void *hashmap_iter_first(struct hashmap *map,
return hashmap_iter_next(iter);
}
+/*
+ * returns a @pointer of @type matching @keyvar, or NULL if nothing found.
+ * @keyvar is a pointer of @type
+ * @member is the name of the "struct hashmap_entry" field in @type
+ */
+#define hashmap_get_entry(map, keyvar, keydata, type, member) \
+ container_of_or_null(hashmap_get(map, &(keyvar)->member, keydata), \
+ type, member)
+
+#define hashmap_get_entry_from_hash(map, hash, keydata, type, member) \
+ 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
+ */
+#define hashmap_get_next_entry(map, var, type, member) \
+ container_of_or_null(hashmap_get_next(map, &(var)->member), \
+ type, 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) \
+ for (; \
+ var; \
+ var = hashmap_get_next_entry(map, var, type, member))
+
/*
* Disable item counting and automatic rehashing when adding/removing items.
*
diff --git a/name-hash.c b/name-hash.c
index dbacb34f50..73b83adf3d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -702,17 +702,16 @@ 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;
+ unsigned int hash = memihash(name, namelen);
lazy_init_name_hash(istate);
- ent = hashmap_get_from_hash(&istate->name_hash,
- memihash(name, namelen), NULL);
- while (ent) {
- ce = container_of(ent, struct cache_entry, ent);
+ 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) {
if (same_name(ce, name, namelen, icase))
return ce;
- 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 d85b8dc58e..e82cbfdee2 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -194,18 +194,16 @@ int cmd__hashmap(int argc, const char **argv)
free(entry);
} else if (!strcmp("get", cmd) && p1) {
- struct hashmap_entry *e;
-
/* lookup entry in hashmap */
- e = hashmap_get_from_hash(&map, hash, p1);
+ entry = hashmap_get_entry_from_hash(&map, hash, p1,
+ struct test_entry, ent);
/* print result */
- if (!e)
+ if (!entry)
puts("NULL");
- while (e) {
- entry = container_of(e, struct test_entry, ent);
+ hashmap_for_each_entry_from(&map, entry,
+ struct test_entry, ent) {
puts(get_value(entry));
- e = hashmap_get_next(&map, e);
}
} else if (!strcmp("remove", cmd) && p1) {
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 13/20] hashmap_get{,_from_hash} return "struct hashmap_entry *"
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (11 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 12/20] hashmap: use *_entry APIs to wrap container_of Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 14/20] hashmap_cmp_fn takes hashmap_entry params Eric Wong
` (9 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Update callers to use hashmap_get_entry, hashmap_get_entry_from_hash
or container_of as appropriate.
This is another step towards eliminating the requirement of
hashmap_entry being the first field in a struct.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
attr.c | 2 +-
blame.c | 11 ++++++++---
builtin/describe.c | 3 ++-
builtin/difftool.c | 2 +-
builtin/fast-export.c | 2 +-
builtin/fetch.c | 11 +++++++----
config.c | 3 ++-
hashmap.c | 7 ++++---
hashmap.h | 12 +++++++-----
merge-recursive.c | 6 ++++--
name-hash.c | 3 ++-
packfile.c | 5 +++--
patch-ids.c | 3 ++-
ref-filter.c | 12 +++++++-----
refs.c | 5 ++++-
remote.c | 7 ++++---
revision.c | 3 ++-
sequencer.c | 7 +++++--
sub-process.c | 3 ++-
submodule-config.c | 6 ++++--
20 files changed, 72 insertions(+), 41 deletions(-)
diff --git a/attr.c b/attr.c
index 9bdef61cc3..4230bee63d 100644
--- a/attr.c
+++ b/attr.c
@@ -101,7 +101,7 @@ static void *attr_hashmap_get(struct attr_hashmap *map,
hashmap_entry_init(&k.ent, memhash(key, keylen));
k.key = key;
k.keylen = keylen;
- e = hashmap_get(&map->map, &k.ent, NULL);
+ e = hashmap_get_entry(&map->map, &k, NULL, struct attr_hash_entry, ent);
return e ? e->value : NULL;
}
diff --git a/blame.c b/blame.c
index 00f8f3fb0a..aa46c7ec52 100644
--- a/blame.c
+++ b/blame.c
@@ -419,7 +419,8 @@ static void get_fingerprint(struct fingerprint *result,
continue;
hashmap_entry_init(&entry->entry, hash);
- found_entry = hashmap_get(&result->map, &entry->entry, NULL);
+ found_entry = hashmap_get_entry(&result->map, entry, NULL,
+ struct fingerprint_entry, entry);
if (found_entry) {
found_entry->count += 1;
} else {
@@ -452,7 +453,9 @@ static int fingerprint_similarity(struct fingerprint *a, struct fingerprint *b)
hashmap_iter_init(&b->map, &iter);
while ((entry_b = hashmap_iter_next(&iter))) {
- if ((entry_a = hashmap_get(&a->map, &entry_b->entry, NULL))) {
+ entry_a = hashmap_get_entry(&a->map, entry_b, NULL,
+ struct fingerprint_entry, entry);
+ if (entry_a) {
intersection += entry_a->count < entry_b->count ?
entry_a->count : entry_b->count;
}
@@ -471,7 +474,9 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b)
hashmap_iter_init(&b->map, &iter);
while ((entry_b = hashmap_iter_next(&iter))) {
- if ((entry_a = hashmap_get(&a->map, &entry_b->entry, NULL))) {
+ entry_a = hashmap_get_entry(&a->map, entry_b, NULL,
+ struct fingerprint_entry, entry);
+ if (entry_a) {
if (entry_a->count <= entry_b->count)
hashmap_remove(&a->map, &entry_b->entry, NULL);
else
diff --git a/builtin/describe.c b/builtin/describe.c
index f5e0a7e033..c6d2386b64 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -76,7 +76,8 @@ static int commit_name_neq(const void *unused_cmp_data,
static inline struct commit_name *find_commit_name(const struct object_id *peeled)
{
- return hashmap_get_from_hash(&names, oidhash(peeled), peeled);
+ return hashmap_get_entry_from_hash(&names, oidhash(peeled), peeled,
+ struct commit_name, entry);
}
static int replace_name(struct commit_name *e,
diff --git a/builtin/difftool.c b/builtin/difftool.c
index f41298d199..fa9c862e3a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -162,7 +162,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
FLEX_ALLOC_STR(e, path, path);
hashmap_entry_init(&e->entry, strhash(path));
- existing = hashmap_get(map, &e->entry, NULL);
+ existing = hashmap_get_entry(map, e, NULL, struct pair_entry, entry);
if (existing) {
free(e);
e = existing;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 192e21dae4..25195badd4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -151,7 +151,7 @@ static const void *anonymize_mem(struct hashmap *map,
hashmap_entry_init(&key.hash, memhash(orig, *len));
key.orig = orig;
key.orig_len = *len;
- ret = hashmap_get(map, &key.hash, NULL);
+ ret = hashmap_get_entry(map, &key, NULL, struct anonymized_entry, hash);
if (!ret) {
ret = xmalloc(sizeof(*ret));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 909dbde909..d06f2b98aa 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -383,8 +383,10 @@ static void find_non_local_tags(const struct ref *refs,
for_each_string_list_item(remote_ref_item, &remote_refs_list) {
const char *refname = remote_ref_item->string;
struct ref *rm;
+ unsigned int hash = strhash(refname);
- item = hashmap_get_from_hash(&remote_refs, strhash(refname), refname);
+ item = hashmap_get_entry_from_hash(&remote_refs, hash, refname,
+ struct refname_hash_entry, ent);
if (!item)
BUG("unseen remote ref?");
@@ -516,10 +518,11 @@ static struct ref *get_ref_map(struct remote *remote,
if (rm->peer_ref) {
const char *refname = rm->peer_ref->name;
struct refname_hash_entry *peer_item;
+ unsigned int hash = strhash(refname);
- peer_item = hashmap_get_from_hash(&existing_refs,
- strhash(refname),
- refname);
+ peer_item = hashmap_get_entry_from_hash(&existing_refs,
+ hash, refname,
+ struct refname_hash_entry, ent);
if (peer_item) {
struct object_id *old_oid = &peer_item->oid;
oidcpy(&rm->peer_ref->old_oid, old_oid);
diff --git a/config.c b/config.c
index 1a1b6675fd..4952d1cc9e 100644
--- a/config.c
+++ b/config.c
@@ -1863,7 +1863,8 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
hashmap_entry_init(&k.ent, strhash(normalized_key));
k.key = normalized_key;
- found_entry = hashmap_get(&cs->config_hash, &k.ent, NULL);
+ found_entry = hashmap_get_entry(&cs->config_hash, &k, NULL,
+ struct config_set_element, ent);
free(normalized_key);
return found_entry;
}
diff --git a/hashmap.c b/hashmap.c
index 22bc7c5b3b..5662bee10a 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -186,8 +186,9 @@ void hashmap_free(struct hashmap *map, int free_entries)
memset(map, 0, sizeof(*map));
}
-void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata)
+struct hashmap_entry *hashmap_get(const struct hashmap *map,
+ const struct hashmap_entry *key,
+ const void *keydata)
{
return *find_entry_ptr(map, key, keydata);
}
@@ -298,7 +299,7 @@ const void *memintern(const void *data, size_t len)
/* lookup interned string in pool */
hashmap_entry_init(&key.ent, memhash(data, len));
key.len = len;
- e = hashmap_get(&map, &key.ent, data);
+ e = hashmap_get_entry(&map, &key, data, struct pool_entry, ent);
if (!e) {
/* not found: create it */
FLEX_ALLOC_MEM(e, data, data, len);
diff --git a/hashmap.h b/hashmap.h
index cd42dcc15c..82ddb0ef41 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -290,8 +290,9 @@ static inline unsigned int hashmap_get_size(struct hashmap *map)
* If an entry with matching hash code is found, `key` and `keydata` are passed
* to `hashmap_cmp_fn` to decide whether the entry matches the key.
*/
-void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata);
+struct hashmap_entry *hashmap_get(const struct hashmap *map,
+ const struct hashmap_entry *key,
+ const void *keydata);
/*
* Returns the hashmap entry for the specified hash code and key data,
@@ -305,9 +306,10 @@ void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
* `entry_or_key` parameter of `hashmap_cmp_fn` points to a hashmap_entry
* structure that should not be used in the comparison.
*/
-static inline void *hashmap_get_from_hash(const struct hashmap *map,
- unsigned int hash,
- const void *keydata)
+static inline struct hashmap_entry *hashmap_get_from_hash(
+ const struct hashmap *map,
+ unsigned int hash,
+ const void *keydata)
{
struct hashmap_entry key;
hashmap_entry_init(&key, hash);
diff --git a/merge-recursive.c b/merge-recursive.c
index a685b4fb69..8274828c4d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -63,7 +63,8 @@ static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
return NULL;
hashmap_entry_init(&key.ent, strhash(dir));
key.dir = dir;
- return hashmap_get(hashmap, &key.ent, NULL);
+ return hashmap_get_entry(hashmap, &key, NULL,
+ struct dir_rename_entry, ent);
}
static int dir_rename_cmp(const void *unused_cmp_data,
@@ -99,7 +100,8 @@ static struct collision_entry *collision_find_entry(struct hashmap *hashmap,
hashmap_entry_init(&key.ent, strhash(target_file));
key.target_file = target_file;
- return hashmap_get(hashmap, &key.ent, NULL);
+ return hashmap_get_entry(hashmap, &key, NULL,
+ struct collision_entry, ent);
}
static int collision_cmp(void *unused_cmp_data,
diff --git a/name-hash.c b/name-hash.c
index 73b83adf3d..aa8253ddd5 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -35,7 +35,8 @@ static struct dir_entry *find_dir_entry__hash(struct index_state *istate,
struct dir_entry key;
hashmap_entry_init(&key.ent, hash);
key.namelen = namelen;
- return hashmap_get(&istate->dir_hash, &key.ent, name);
+ return hashmap_get_entry(&istate->dir_hash, &key, name,
+ struct dir_entry, ent);
}
static struct dir_entry *find_dir_entry(struct index_state *istate,
diff --git a/packfile.c b/packfile.c
index 3edd648de0..f2aa34bb49 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1381,7 +1381,7 @@ static unsigned int pack_entry_hash(struct packed_git *p, off_t base_offset)
static struct delta_base_cache_entry *
get_delta_base_cache_entry(struct packed_git *p, off_t base_offset)
{
- struct hashmap_entry entry;
+ struct hashmap_entry entry, *e;
struct delta_base_cache_key key;
if (!delta_base_cache.cmpfn)
@@ -1390,7 +1390,8 @@ get_delta_base_cache_entry(struct packed_git *p, off_t base_offset)
hashmap_entry_init(&entry, pack_entry_hash(p, base_offset));
key.p = p;
key.base_offset = base_offset;
- return hashmap_get(&delta_base_cache, &entry, &key);
+ e = hashmap_get(&delta_base_cache, &entry, &key);
+ return e ? container_of(e, struct delta_base_cache_entry, ent) : NULL;
}
static int delta_base_cache_key_eq(const struct delta_base_cache_key *a,
diff --git a/patch-ids.c b/patch-ids.c
index 437f29e42c..176c47d967 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -99,7 +99,8 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
if (init_patch_id_entry(&patch, commit, ids))
return NULL;
- return hashmap_get(&ids->patches, &patch.ent, NULL);
+ return hashmap_get_entry(&ids->patches, &patch, NULL,
+ struct patch_id, ent);
}
struct patch_id *add_commit_patch_id(struct commit *commit,
diff --git a/ref-filter.c b/ref-filter.c
index d939ebc6bb..9999426914 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1585,18 +1585,20 @@ static void lazy_init_worktree_map(void)
static char *get_worktree_path(const struct used_atom *atom, const struct ref_array_item *ref)
{
- struct hashmap_entry entry;
+ struct hashmap_entry entry, *e;
struct ref_to_worktree_entry *lookup_result;
lazy_init_worktree_map();
hashmap_entry_init(&entry, strhash(ref->refname));
- lookup_result = hashmap_get(&(ref_to_worktree_map.map), &entry, ref->refname);
+ e = hashmap_get(&(ref_to_worktree_map.map), &entry, ref->refname);
- if (lookup_result)
- return xstrdup(lookup_result->wt->path);
- else
+ if (!e)
return xstrdup("");
+
+ lookup_result = container_of(e, struct ref_to_worktree_entry, ent);
+
+ return xstrdup(lookup_result->wt->path);
}
/*
diff --git a/refs.c b/refs.c
index 3e55031256..43a95105f1 100644
--- a/refs.c
+++ b/refs.c
@@ -1815,12 +1815,15 @@ static struct ref_store *lookup_ref_store_map(struct hashmap *map,
const char *name)
{
struct ref_store_hash_entry *entry;
+ unsigned int hash;
if (!map->tablesize)
/* It's initialized on demand in register_ref_store(). */
return NULL;
- entry = hashmap_get_from_hash(map, strhash(name), name);
+ hash = strhash(name);
+ entry = hashmap_get_entry_from_hash(map, hash, name,
+ struct ref_store_hash_entry, ent);
return entry ? entry->refs : NULL;
}
diff --git a/remote.c b/remote.c
index 8ca23d95dc..ed95ae6ed6 100644
--- a/remote.c
+++ b/remote.c
@@ -135,7 +135,7 @@ static struct remote *make_remote(const char *name, int len)
{
struct remote *ret, *replaced;
struct remotes_hash_key lookup;
- struct hashmap_entry lookup_entry;
+ struct hashmap_entry lookup_entry, *e;
if (!len)
len = strlen(name);
@@ -145,8 +145,9 @@ static struct remote *make_remote(const char *name, int len)
lookup.len = len;
hashmap_entry_init(&lookup_entry, memhash(name, len));
- if ((ret = hashmap_get(&remotes_hash, &lookup_entry, &lookup)) != NULL)
- return ret;
+ e = hashmap_get(&remotes_hash, &lookup_entry, &lookup);
+ if (e)
+ return container_of(e, struct remote, ent);
ret = xcalloc(1, sizeof(struct remote));
ret->prune = -1; /* unspecified */
diff --git a/revision.c b/revision.c
index a7e2339064..d5f534209d 100644
--- a/revision.c
+++ b/revision.c
@@ -147,7 +147,8 @@ static void paths_and_oids_insert(struct hashmap *map,
key.path = (char *)path;
oidset_init(&key.trees, 0);
- entry = hashmap_get(map, &key.ent, NULL);
+ entry = hashmap_get_entry(map, &key, NULL,
+ struct path_and_oids_entry, ent);
if (!entry) {
entry = xcalloc(1, sizeof(struct path_and_oids_entry));
hashmap_entry_init(&entry->ent, hash);
diff --git a/sequencer.c b/sequencer.c
index b4ef70e260..aea2cb12cc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5217,8 +5217,11 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
break;
}
- if ((entry = hashmap_get_from_hash(&subject2item,
- strhash(p), p)))
+ entry = hashmap_get_entry_from_hash(&subject2item,
+ strhash(p), p,
+ struct subject2item_entry,
+ entry);
+ if (entry)
/* found by title */
i2 = entry->i;
else if (!strchr(p, ' ') &&
diff --git a/sub-process.c b/sub-process.c
index 99fccef592..f2fcc16c3e 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -22,7 +22,8 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch
hashmap_entry_init(&key.ent, strhash(cmd));
key.cmd = cmd;
- return hashmap_get(hashmap, &key.ent, NULL);
+ return hashmap_get_entry(hashmap, &key, NULL,
+ struct subprocess_entry, ent);
}
int subprocess_read_status(int fd, struct strbuf *status)
diff --git a/submodule-config.c b/submodule-config.c
index 9248c5ea5b..b031884789 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -166,7 +166,8 @@ static const struct submodule *cache_lookup_path(struct submodule_cache *cache,
hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
- entry = hashmap_get(&cache->for_path, &key.ent, NULL);
+ entry = hashmap_get_entry(&cache->for_path, &key, NULL,
+ struct submodule_entry, ent);
if (entry)
return entry->config;
return NULL;
@@ -186,7 +187,8 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
- entry = hashmap_get(&cache->for_name, &key.ent, NULL);
+ entry = hashmap_get_entry(&cache->for_name, &key, NULL,
+ struct submodule_entry, ent);
if (entry)
return entry->config;
return NULL;
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 14/20] hashmap_cmp_fn takes hashmap_entry params
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (12 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 13/20] hashmap_get{,_from_hash} return "struct hashmap_entry *" Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 15/20] hashmap: use *_entry APIs for iteration Eric Wong
` (8 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Another step in eliminating the requirement of hashmap_entry
being the first member of a struct.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
attr.c | 10 ++++++----
builtin/describe.c | 10 ++++++----
builtin/difftool.c | 31 +++++++++++++++++++------------
builtin/fast-export.c | 9 +++++++--
builtin/fetch.c | 9 +++++----
config.c | 10 ++++++----
diff.c | 12 +++++++-----
hashmap.c | 17 +++++++++++------
hashmap.h | 13 +++++++++----
merge-recursive.c | 33 +++++++++++++++++++++------------
name-hash.c | 21 +++++++++++++--------
oidmap.c | 14 +++++++++-----
packfile.c | 9 +++++++--
patch-ids.c | 10 ++++++----
ref-filter.c | 11 +++++++----
refs.c | 11 ++++++++---
remote.c | 10 ++++++----
revision.c | 11 ++++++++---
sequencer.c | 24 +++++++++++++++++-------
sub-process.c | 10 ++++++----
sub-process.h | 4 ++--
submodule-config.c | 20 ++++++++++++--------
t/helper/test-hashmap.c | 10 ++++++----
23 files changed, 204 insertions(+), 115 deletions(-)
diff --git a/attr.c b/attr.c
index 4230bee63d..6053481610 100644
--- a/attr.c
+++ b/attr.c
@@ -70,12 +70,14 @@ struct attr_hash_entry {
/* attr_hashmap comparison function */
static int attr_hash_entry_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct attr_hash_entry *a = entry;
- const struct attr_hash_entry *b = entry_or_key;
+ const struct attr_hash_entry *a, *b;
+
+ a = container_of(eptr, const struct attr_hash_entry, ent);
+ b = container_of(entry_or_key, const struct attr_hash_entry, ent);
return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
}
diff --git a/builtin/describe.c b/builtin/describe.c
index c6d2386b64..e9267b5c9c 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -64,12 +64,14 @@ static const char *prio_names[] = {
};
static int commit_name_neq(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *peeled)
{
- const struct commit_name *cn1 = entry;
- const struct commit_name *cn2 = entry_or_key;
+ const struct commit_name *cn1, *cn2;
+
+ cn1 = container_of(eptr, const struct commit_name, entry);
+ cn2 = container_of(entry_or_key, const struct commit_name, entry);
return !oideq(&cn1->peeled, peeled ? peeled : &cn2->peeled);
}
diff --git a/builtin/difftool.c b/builtin/difftool.c
index fa9c862e3a..4a37b3edee 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -125,12 +125,15 @@ struct working_tree_entry {
};
static int working_tree_entry_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct working_tree_entry *a = entry;
- const struct working_tree_entry *b = entry_or_key;
+ const struct working_tree_entry *a, *b;
+
+ a = container_of(eptr, const struct working_tree_entry, entry);
+ b = container_of(entry_or_key, const struct working_tree_entry, entry);
+
return strcmp(a->path, b->path);
}
@@ -145,12 +148,14 @@ struct pair_entry {
};
static int pair_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct pair_entry *a = entry;
- const struct pair_entry *b = entry_or_key;
+ const struct pair_entry *a, *b;
+
+ a = container_of(eptr, const struct pair_entry, entry);
+ b = container_of(entry_or_key, const struct pair_entry, entry);
return strcmp(a->path, b->path);
}
@@ -179,12 +184,14 @@ struct path_entry {
};
static int path_entry_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *key)
{
- const struct path_entry *a = entry;
- const struct path_entry *b = entry_or_key;
+ const struct path_entry *a, *b;
+
+ a = container_of(eptr, const struct path_entry, entry);
+ b = container_of(entry_or_key, const struct path_entry, entry);
return strcmp(a->path, key ? key : b->path);
}
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 25195badd4..ef0578bf90 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -126,10 +126,15 @@ struct anonymized_entry {
};
static int anonymized_entry_cmp(const void *unused_cmp_data,
- const void *va, const void *vb,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct anonymized_entry *a = va, *b = vb;
+ const struct anonymized_entry *a, *b;
+
+ a = container_of(eptr, const struct anonymized_entry, hash);
+ b = container_of(entry_or_key, const struct anonymized_entry, hash);
+
return a->orig_len != b->orig_len ||
memcmp(a->orig, b->orig, a->orig_len);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d06f2b98aa..476c2416e3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -258,13 +258,14 @@ struct refname_hash_entry {
};
static int refname_hash_entry_cmp(const void *hashmap_cmp_fn_data,
- const void *e1_,
- const void *e2_,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
- const struct refname_hash_entry *e1 = e1_;
- const struct refname_hash_entry *e2 = e2_;
+ const struct refname_hash_entry *e1, *e2;
+ e1 = container_of(eptr, const struct refname_hash_entry, ent);
+ e2 = container_of(entry_or_key, const struct refname_hash_entry, ent);
return strcmp(e1->refname, keydata ? keydata : e2->refname);
}
diff --git a/config.c b/config.c
index 4952d1cc9e..33043ee73c 100644
--- a/config.c
+++ b/config.c
@@ -1914,12 +1914,14 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
}
static int config_set_element_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct config_set_element *e1 = entry;
- const struct config_set_element *e2 = entry_or_key;
+ const struct config_set_element *e1, *e2;
+
+ e1 = container_of(eptr, const struct config_set_element, ent);
+ e2 = container_of(entry_or_key, const struct config_set_element, ent);
return strcmp(e1->key, e2->key);
}
diff --git a/diff.c b/diff.c
index 66cdf4e9ca..5eaf689fcc 100644
--- a/diff.c
+++ b/diff.c
@@ -933,16 +933,18 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,
}
static int moved_entry_cmp(const void *hashmap_cmp_fn_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
const struct diff_options *diffopt = hashmap_cmp_fn_data;
- const struct moved_entry *a = entry;
- const struct moved_entry *b = entry_or_key;
+ const struct moved_entry *a, *b;
unsigned flags = diffopt->color_moved_ws_handling
& XDF_WHITESPACE_FLAGS;
+ a = container_of(eptr, const struct moved_entry, ent);
+ b = container_of(entry_or_key, const struct moved_entry, ent);
+
if (diffopt->color_moved_ws_handling &
COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)
/*
@@ -1019,7 +1021,7 @@ static void pmb_advance_or_null(struct diff_options *o,
struct moved_entry *prev = pmb[i].match;
struct moved_entry *cur = (prev && prev->next_line) ?
prev->next_line : NULL;
- if (cur && !hm->cmpfn(o, cur, match, NULL)) {
+ if (cur && !hm->cmpfn(o, &cur->ent, &match->ent, NULL)) {
pmb[i].match = cur;
} else {
pmb[i].match = NULL;
diff --git a/hashmap.c b/hashmap.c
index 5662bee10a..64f6accfff 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -140,8 +140,8 @@ static inline struct hashmap_entry **find_entry_ptr(const struct hashmap *map,
}
static int always_equal(const void *unused_cmp_data,
- const void *unused1,
- const void *unused2,
+ const struct hashmap_entry *unused1,
+ const struct hashmap_entry *unused2,
const void *unused_keydata)
{
return 0;
@@ -279,10 +279,15 @@ struct pool_entry {
};
static int pool_entry_cmp(const void *unused_cmp_data,
- const struct pool_entry *e1,
- const struct pool_entry *e2,
- const unsigned char *keydata)
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
+ const void *keydata)
{
+ const struct pool_entry *e1, *e2;
+
+ e1 = container_of(eptr, const struct pool_entry, ent);
+ e2 = container_of(entry_or_key, const struct pool_entry, ent);
+
return e1->data != keydata &&
(e1->len != e2->len || memcmp(e1->data, keydata, e1->len));
}
@@ -294,7 +299,7 @@ const void *memintern(const void *data, size_t len)
/* initialize string pool hashmap */
if (!map.tablesize)
- hashmap_init(&map, (hashmap_cmp_fn) pool_entry_cmp, NULL, 0);
+ hashmap_init(&map, pool_entry_cmp, NULL, 0);
/* lookup interned string in pool */
hashmap_entry_init(&key.ent, memhash(data, len));
diff --git a/hashmap.h b/hashmap.h
index 82ddb0ef41..8f5c163d56 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -21,12 +21,16 @@
* #define COMPARE_VALUE 1
*
* static int long2string_cmp(const void *hashmap_cmp_fn_data,
- * const struct long2string *e1,
- * const struct long2string *e2,
+ * const struct hashmap_entry *eptr,
+ * const struct hashmap_entry *entry_or_key,
* const void *keydata)
* {
* const char *string = keydata;
* unsigned flags = *(unsigned *)hashmap_cmp_fn_data;
+ * const struct long2string *e1, *e2;
+ *
+ * e1 = container_of(eptr, const struct long2string, ent);
+ * e2 = container_of(entry_or_key, const struct long2string, ent);
*
* if (flags & COMPARE_VALUE)
* return e1->key != e2->key ||
@@ -41,7 +45,7 @@
* char value[255], action[32];
* unsigned flags = 0;
*
- * hashmap_init(&map, (hashmap_cmp_fn) long2string_cmp, &flags, 0);
+ * hashmap_init(&map, long2string_cmp, &flags, 0);
*
* while (scanf("%s %ld %s", action, &key, value)) {
*
@@ -172,7 +176,8 @@ struct hashmap_entry {
* The `hashmap_cmp_fn_data` entry is the pointer given in the init function.
*/
typedef int (*hashmap_cmp_fn)(const void *hashmap_cmp_fn_data,
- const void *entry, const void *entry_or_key,
+ const struct hashmap_entry *entry,
+ const struct hashmap_entry *entry_or_key,
const void *keydata);
/*
diff --git a/merge-recursive.c b/merge-recursive.c
index 8274828c4d..b06e9f7f0b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -35,14 +35,16 @@ struct path_hashmap_entry {
};
static int path_hashmap_cmp(const void *cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
- const struct path_hashmap_entry *a = entry;
- const struct path_hashmap_entry *b = entry_or_key;
+ const struct path_hashmap_entry *a, *b;
const char *key = keydata;
+ a = container_of(eptr, const struct path_hashmap_entry, e);
+ b = container_of(entry_or_key, const struct path_hashmap_entry, e);
+
if (ignore_case)
return strcasecmp(a->path, key ? key : b->path);
else
@@ -68,12 +70,14 @@ static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
}
static int dir_rename_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct dir_rename_entry *e1 = entry;
- const struct dir_rename_entry *e2 = entry_or_key;
+ const struct dir_rename_entry *e1, *e2;
+
+ e1 = container_of(eptr, const struct dir_rename_entry, ent);
+ e2 = container_of(entry_or_key, const struct dir_rename_entry, ent);
return strcmp(e1->dir, e2->dir);
}
@@ -104,17 +108,22 @@ static struct collision_entry *collision_find_entry(struct hashmap *hashmap,
struct collision_entry, ent);
}
-static int collision_cmp(void *unused_cmp_data,
- const struct collision_entry *e1,
- const struct collision_entry *e2,
+static int collision_cmp(const void *unused_cmp_data,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
+ const struct collision_entry *e1, *e2;
+
+ e1 = container_of(eptr, const struct collision_entry, ent);
+ e2 = container_of(entry_or_key, const struct collision_entry, ent);
+
return strcmp(e1->target_file, e2->target_file);
}
static void collision_init(struct hashmap *map)
{
- hashmap_init(map, (hashmap_cmp_fn) collision_cmp, NULL, 0);
+ hashmap_init(map, collision_cmp, NULL, 0);
}
static void flush_output(struct merge_options *opt)
diff --git a/name-hash.c b/name-hash.c
index aa8253ddd5..85a1ce982c 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -17,14 +17,16 @@ struct dir_entry {
};
static int dir_entry_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
- const struct dir_entry *e1 = entry;
- const struct dir_entry *e2 = entry_or_key;
+ const struct dir_entry *e1, *e2;
const char *name = keydata;
+ e1 = container_of(eptr, const struct dir_entry, ent);
+ e2 = container_of(entry_or_key, const struct dir_entry, ent);
+
return e1->namelen != e2->namelen || strncasecmp(e1->name,
name ? name : e2->name, e1->namelen);
}
@@ -115,12 +117,15 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
}
static int cache_entry_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *remove)
{
- const struct cache_entry *ce1 = entry;
- const struct cache_entry *ce2 = entry_or_key;
+ const struct cache_entry *ce1, *ce2;
+
+ ce1 = container_of(eptr, const struct cache_entry, ent);
+ ce2 = container_of(entry_or_key, const struct cache_entry, ent);
+
/*
* For remove_name_hash, find the exact entry (pointer equality); for
* index_file_exists, find all entries with matching hash code and
diff --git a/oidmap.c b/oidmap.c
index cd22b3a8bf..4942599391 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -2,14 +2,18 @@
#include "oidmap.h"
static int oidmap_neq(const void *hashmap_cmp_fn_data,
- const void *entry, const void *entry_or_key,
+ const struct hashmap_entry *e1,
+ const struct hashmap_entry *e2,
const void *keydata)
{
- const struct oidmap_entry *entry_ = entry;
+ const struct oidmap_entry *a, *b;
+
+ a = container_of(e1, const struct oidmap_entry, internal_entry);
+ b = container_of(e2, const struct oidmap_entry, internal_entry);
+
if (keydata)
- return !oideq(&entry_->oid, (const struct object_id *) keydata);
- return !oideq(&entry_->oid,
- &((const struct oidmap_entry *) entry_or_key)->oid);
+ return !oideq(&a->oid, (const struct object_id *) keydata);
+ return !oideq(&a->oid, &b->oid);
}
void oidmap_init(struct oidmap *map, size_t initial_size)
diff --git a/packfile.c b/packfile.c
index f2aa34bb49..675d5f2287 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1401,11 +1401,16 @@ static int delta_base_cache_key_eq(const struct delta_base_cache_key *a,
}
static int delta_base_cache_hash_cmp(const void *unused_cmp_data,
- const void *va, const void *vb,
+ const struct hashmap_entry *va,
+ const struct hashmap_entry *vb,
const void *vkey)
{
- const struct delta_base_cache_entry *a = va, *b = vb;
+ const struct delta_base_cache_entry *a, *b;
const struct delta_base_cache_key *key = vkey;
+
+ a = container_of(va, const struct delta_base_cache_entry, ent);
+ b = container_of(vb, const struct delta_base_cache_entry, ent);
+
if (key)
return !delta_base_cache_key_eq(&a->key, key);
else
diff --git a/patch-ids.c b/patch-ids.c
index 176c47d967..75f8c9f1a1 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -36,14 +36,16 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
* any significance; only that it is non-zero matters.
*/
static int patch_id_neq(const void *cmpfn_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
/* NEEDSWORK: const correctness? */
struct diff_options *opt = (void *)cmpfn_data;
- struct patch_id *a = (void *)entry;
- struct patch_id *b = (void *)entry_or_key;
+ struct patch_id *a, *b;
+
+ a = container_of(eptr, struct patch_id, ent);
+ b = container_of(entry_or_key, struct patch_id, ent);
if (is_null_oid(&a->patch_id) &&
commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))
diff --git a/ref-filter.c b/ref-filter.c
index 9999426914..4613df8826 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -84,12 +84,15 @@ struct ref_to_worktree_entry {
};
static int ref_to_worktree_map_cmpfnc(const void *unused_lookupdata,
- const void *existing_hashmap_entry_to_test,
- const void *key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *kptr,
const void *keydata_aka_refname)
{
- const struct ref_to_worktree_entry *e = existing_hashmap_entry_to_test;
- const struct ref_to_worktree_entry *k = key;
+ const struct ref_to_worktree_entry *e, *k;
+
+ e = container_of(eptr, const struct ref_to_worktree_entry, ent);
+ k = container_of(kptr, const struct ref_to_worktree_entry, ent);
+
return strcmp(e->wt->head_ref,
keydata_aka_refname ? keydata_aka_refname : k->wt->head_ref);
}
diff --git a/refs.c b/refs.c
index 43a95105f1..2d3eb40f39 100644
--- a/refs.c
+++ b/refs.c
@@ -1781,11 +1781,16 @@ struct ref_store_hash_entry
};
static int ref_store_hash_cmp(const void *unused_cmp_data,
- const void *entry, const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
- const struct ref_store_hash_entry *e1 = entry, *e2 = entry_or_key;
- const char *name = keydata ? keydata : e2->name;
+ const struct ref_store_hash_entry *e1, *e2;
+ const char *name;
+
+ e1 = container_of(eptr, const struct ref_store_hash_entry, ent);
+ e2 = container_of(entry_or_key, const struct ref_store_hash_entry, ent);
+ name = keydata ? keydata : e2->name;
return strcmp(e1->name, name);
}
diff --git a/remote.c b/remote.c
index ed95ae6ed6..fa9cadcfbd 100644
--- a/remote.c
+++ b/remote.c
@@ -111,14 +111,16 @@ struct remotes_hash_key {
};
static int remotes_hash_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
- const struct remote *a = entry;
- const struct remote *b = entry_or_key;
+ const struct remote *a, *b;
const struct remotes_hash_key *key = keydata;
+ a = container_of(eptr, const struct remote, ent);
+ b = container_of(entry_or_key, const struct remote, ent);
+
if (key)
return strncmp(a->name, key->str, key->len) || a->name[key->len];
else
diff --git a/revision.c b/revision.c
index d5f534209d..f32fbc5e2e 100644
--- a/revision.c
+++ b/revision.c
@@ -107,16 +107,21 @@ struct path_and_oids_entry {
};
static int path_and_oids_cmp(const void *hashmap_cmp_fn_data,
- const struct path_and_oids_entry *e1,
- const struct path_and_oids_entry *e2,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
+ const struct path_and_oids_entry *e1, *e2;
+
+ e1 = container_of(eptr, const struct path_and_oids_entry, ent);
+ e2 = container_of(entry_or_key, const struct path_and_oids_entry, ent);
+
return strcmp(e1->path, e2->path);
}
static void paths_and_oids_init(struct hashmap *map)
{
- hashmap_init(map, (hashmap_cmp_fn) path_and_oids_cmp, NULL, 0);
+ hashmap_init(map, path_and_oids_cmp, NULL, 0);
}
static void paths_and_oids_clear(struct hashmap *map)
diff --git a/sequencer.c b/sequencer.c
index aea2cb12cc..b3e7319b55 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4440,9 +4440,14 @@ struct labels_entry {
char label[FLEX_ARRAY];
};
-static int labels_cmp(const void *fndata, const struct labels_entry *a,
- const struct labels_entry *b, const void *key)
+static int labels_cmp(const void *fndata, const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key, const void *key)
{
+ const struct labels_entry *a, *b;
+
+ a = container_of(eptr, const struct labels_entry, entry);
+ b = container_of(entry_or_key, const struct labels_entry, entry);
+
return key ? strcmp(a->label, key) : strcmp(a->label, b->label);
}
@@ -4573,7 +4578,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
oidmap_init(&commit2todo, 0);
oidmap_init(&state.commit2label, 0);
- hashmap_init(&state.labels, (hashmap_cmp_fn) labels_cmp, NULL, 0);
+ hashmap_init(&state.labels, labels_cmp, NULL, 0);
strbuf_init(&state.buf, 32);
if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
@@ -5138,9 +5143,15 @@ struct subject2item_entry {
};
static int subject2item_cmp(const void *fndata,
- const struct subject2item_entry *a,
- const struct subject2item_entry *b, const void *key)
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
+ const void *key)
{
+ const struct subject2item_entry *a, *b;
+
+ a = container_of(eptr, const struct subject2item_entry, entry);
+ b = container_of(entry_or_key, const struct subject2item_entry, entry);
+
return key ? strcmp(a->subject, key) : strcmp(a->subject, b->subject);
}
@@ -5173,8 +5184,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
* In that case, last[i] will indicate the index of the latest item to
* be moved to appear after the i'th.
*/
- hashmap_init(&subject2item, (hashmap_cmp_fn) subject2item_cmp,
- NULL, todo_list->nr);
+ hashmap_init(&subject2item, subject2item_cmp, NULL, todo_list->nr);
ALLOC_ARRAY(next, todo_list->nr);
ALLOC_ARRAY(tail, todo_list->nr);
ALLOC_ARRAY(subjects, todo_list->nr);
diff --git a/sub-process.c b/sub-process.c
index f2fcc16c3e..ad94f72665 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -6,12 +6,14 @@
#include "pkt-line.h"
int cmd2process_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct subprocess_entry *e1 = entry;
- const struct subprocess_entry *e2 = entry_or_key;
+ const struct subprocess_entry *e1, *e2;
+
+ e1 = container_of(eptr, const struct subprocess_entry, ent);
+ e2 = container_of(entry_or_key, const struct subprocess_entry, ent);
return strcmp(e1->cmd, e2->cmd);
}
diff --git a/sub-process.h b/sub-process.h
index 5c182fad98..0d12708b8c 100644
--- a/sub-process.h
+++ b/sub-process.h
@@ -43,8 +43,8 @@ struct subprocess_capability {
/* Function to test two subprocess hashmap entries for equality. */
int cmd2process_cmp(const void *unused_cmp_data,
- const void *e1,
- const void *e2,
+ const struct hashmap_entry *e,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata);
/*
diff --git a/submodule-config.c b/submodule-config.c
index b031884789..5463729ab8 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -38,24 +38,28 @@ enum lookup_type {
};
static int config_path_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct submodule_entry *a = entry;
- const struct submodule_entry *b = entry_or_key;
+ const struct submodule_entry *a, *b;
+
+ a = container_of(eptr, const struct submodule_entry, ent);
+ b = container_of(entry_or_key, const struct submodule_entry, ent);
return strcmp(a->config->path, b->config->path) ||
!oideq(&a->config->gitmodules_oid, &b->config->gitmodules_oid);
}
static int config_name_cmp(const void *unused_cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *unused_keydata)
{
- const struct submodule_entry *a = entry;
- const struct submodule_entry *b = entry_or_key;
+ const struct submodule_entry *a, *b;
+
+ a = container_of(eptr, const struct submodule_entry, ent);
+ b = container_of(entry_or_key, const struct submodule_entry, ent);
return strcmp(a->config->name, b->config->name) ||
!oideq(&a->config->gitmodules_oid, &b->config->gitmodules_oid);
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index e82cbfdee2..56846da64c 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -16,15 +16,17 @@ static const char *get_value(const struct test_entry *e)
}
static int test_entry_cmp(const void *cmp_data,
- const void *entry,
- const void *entry_or_key,
+ const struct hashmap_entry *eptr,
+ const struct hashmap_entry *entry_or_key,
const void *keydata)
{
const int ignore_case = cmp_data ? *((int *)cmp_data) : 0;
- const struct test_entry *e1 = entry;
- const struct test_entry *e2 = entry_or_key;
+ const struct test_entry *e1, *e2;
const char *key = keydata;
+ e1 = container_of(eptr, const struct test_entry, ent);
+ e2 = container_of(entry_or_key, const struct test_entry, ent);
+
if (ignore_case)
return strcasecmp(e1->key, key ? key : e2->key);
else
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 15/20] hashmap: use *_entry APIs for iteration
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (13 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 14/20] hashmap_cmp_fn takes hashmap_entry params Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 16/20] hashmap: hashmap_{put,remove} return hashmap_entry * Eric Wong
` (7 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Inspired by list_for_each_entry in the Linux kernel.
Once again, these are somewhat compromised usability-wise
by compilers lacking __typeof__ support.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
attr.c | 5 +++--
blame.c | 10 ++++++----
builtin/describe.c | 4 ++--
builtin/difftool.c | 8 ++++----
config.c | 5 +++--
hashmap.c | 2 +-
hashmap.h | 15 +++++++++++++--
merge-recursive.c | 25 +++++++++++++++----------
oidmap.h | 6 ++++--
revision.c | 10 ++++++----
submodule-config.c | 8 +++++---
t/helper/test-hashmap.c | 7 ++++---
t/helper/test-lazy-init-name-hash.c | 12 ++++--------
13 files changed, 70 insertions(+), 47 deletions(-)
diff --git a/attr.c b/attr.c
index 6053481610..ca8be46e8e 100644
--- a/attr.c
+++ b/attr.c
@@ -163,12 +163,13 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
if (size != check->all_attrs_nr) {
struct attr_hash_entry *e;
struct hashmap_iter iter;
- hashmap_iter_init(&map->map, &iter);
REALLOC_ARRAY(check->all_attrs, size);
check->all_attrs_nr = size;
- while ((e = hashmap_iter_next(&iter))) {
+ 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 aa46c7ec52..3d8accf902 100644
--- a/blame.c
+++ b/blame.c
@@ -450,9 +450,9 @@ static int fingerprint_similarity(struct fingerprint *a, struct fingerprint *b)
struct hashmap_iter iter;
const struct fingerprint_entry *entry_a, *entry_b;
- hashmap_iter_init(&b->map, &iter);
-
- while ((entry_b = hashmap_iter_next(&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);
if (entry_a) {
@@ -473,7 +473,9 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b)
hashmap_iter_init(&b->map, &iter);
- while ((entry_b = hashmap_iter_next(&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);
if (entry_a) {
diff --git a/builtin/describe.c b/builtin/describe.c
index e9267b5c9c..8cf2cd992d 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -333,8 +333,8 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)
struct commit_name *n;
init_commit_names(&commit_names);
- n = hashmap_iter_first(&names, &iter);
- for (; n; n = hashmap_iter_next(&iter)) {
+ hashmap_for_each_entry(&names, &iter, n, struct commit_name,
+ entry /* member name */) {
c = lookup_commit_reference_gently(the_repository,
&n->peeled, 1);
if (c)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4a37b3edee..dd94179b68 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -538,8 +538,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
* temporary file to both the left and right directories to show the
* change in the recorded SHA1 for the submodule.
*/
- hashmap_iter_init(&submodules, &iter);
- while ((entry = hashmap_iter_next(&iter))) {
+ hashmap_for_each_entry(&submodules, &iter, entry,
+ struct pair_entry, entry /* member name */) {
if (*entry->left) {
add_path(&ldir, ldir_len, entry->path);
ensure_leading_directories(ldir.buf);
@@ -557,8 +557,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
* shows only the link itself, not the contents of the link target.
* This loop replicates that behavior.
*/
- hashmap_iter_init(&symlinks2, &iter);
- while ((entry = hashmap_iter_next(&iter))) {
+ hashmap_for_each_entry(&symlinks2, &iter, entry,
+ struct pair_entry, 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 33043ee73c..8433f74371 100644
--- a/config.c
+++ b/config.c
@@ -1942,8 +1942,9 @@ void git_configset_clear(struct config_set *cs)
if (!cs->hash_initialized)
return;
- hashmap_iter_init(&cs->config_hash, &iter);
- while ((entry = hashmap_iter_next(&iter))) {
+ 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/hashmap.c b/hashmap.c
index 64f6accfff..deb5fdf28c 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -256,7 +256,7 @@ void hashmap_iter_init(struct hashmap *map, struct hashmap_iter *iter)
iter->next = NULL;
}
-void *hashmap_iter_next(struct hashmap_iter *iter)
+struct hashmap_entry *hashmap_iter_next(struct hashmap_iter *iter)
{
struct hashmap_entry *current = iter->next;
for (;;) {
diff --git a/hashmap.h b/hashmap.h
index 8f5c163d56..8d4b3907b4 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -382,16 +382,27 @@ struct hashmap_iter {
void hashmap_iter_init(struct hashmap *map, struct hashmap_iter *iter);
/* Returns the next hashmap_entry, or NULL if there are no more entries. */
-void *hashmap_iter_next(struct hashmap_iter *iter);
+struct hashmap_entry *hashmap_iter_next(struct hashmap_iter *iter);
/* Initializes the iterator and returns the first entry, if any. */
-static inline void *hashmap_iter_first(struct hashmap *map,
+static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map,
struct hashmap_iter *iter)
{
hashmap_iter_init(map, iter);
return hashmap_iter_next(iter);
}
+#define hashmap_iter_next_entry(iter, type, member) \
+ container_of_or_null(hashmap_iter_next(iter), type, member)
+
+#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); \
+ var; \
+ var = hashmap_iter_next_entry(iter, type, member))
+
/*
* returns a @pointer of @type matching @keyvar, or NULL if nothing found.
* @keyvar is a pointer of @type
diff --git a/merge-recursive.c b/merge-recursive.c
index b06e9f7f0b..73c7750448 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2135,8 +2135,9 @@ static void handle_directory_level_conflicts(struct merge_options *opt,
struct string_list remove_from_head = STRING_LIST_INIT_NODUP;
struct string_list remove_from_merge = STRING_LIST_INIT_NODUP;
- hashmap_iter_init(dir_re_head, &iter);
- while ((head_ent = hashmap_iter_next(&iter))) {
+ 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 &&
!head_ent->non_unique_new_dir &&
@@ -2160,8 +2161,9 @@ static void handle_directory_level_conflicts(struct merge_options *opt,
remove_hashmap_entries(dir_re_head, &remove_from_head);
remove_hashmap_entries(dir_re_merge, &remove_from_merge);
- hashmap_iter_init(dir_re_merge, &iter);
- while ((merge_ent = hashmap_iter_next(&iter))) {
+ 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)) {
/* 2. This wasn't a directory rename after all */
@@ -2265,8 +2267,9 @@ static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)
* we set non_unique_new_dir. Once we've determined the winner (or
* that there is no winner), we no longer need possible_new_dirs.
*/
- hashmap_iter_init(dir_renames, &iter);
- while ((entry = hashmap_iter_next(&iter))) {
+ hashmap_for_each_entry(dir_renames, &iter, entry,
+ struct dir_rename_entry,
+ ent /* member name */) {
int max = 0;
int bad_max = 0;
char *best = NULL;
@@ -2624,8 +2627,9 @@ static struct string_list *get_renames(struct merge_options *opt,
entries);
}
- hashmap_iter_init(&collisions, &iter);
- while ((e = hashmap_iter_next(&iter))) {
+ hashmap_for_each_entry(&collisions, &iter, e,
+ struct collision_entry,
+ ent /* member name */) {
free(e->target_file);
string_list_clear(&e->source_files, 0);
}
@@ -2842,8 +2846,9 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs,
struct hashmap_iter iter;
struct dir_rename_entry *e;
- hashmap_iter_init(dir_renames, &iter);
- while ((e = hashmap_iter_next(&iter))) {
+ hashmap_for_each_entry(dir_renames, &iter, e,
+ struct dir_rename_entry,
+ ent /* member name */) {
free(e->dir);
strbuf_release(&e->new_dir);
/* possible_new_dirs already cleared in get_directory_renames */
diff --git a/oidmap.h b/oidmap.h
index 7a939461ff..c66a83ab1d 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -78,14 +78,16 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter
static inline void *oidmap_iter_next(struct oidmap_iter *iter)
{
- return hashmap_iter_next(&iter->h_iter);
+ /* TODO: this API could be reworked to do compile-time type checks */
+ return (void *)hashmap_iter_next(&iter->h_iter);
}
static inline void *oidmap_iter_first(struct oidmap *map,
struct oidmap_iter *iter)
{
oidmap_iter_init(map, iter);
- return oidmap_iter_next(iter);
+ /* TODO: this API could be reworked to do compile-time type checks */
+ return (void *)oidmap_iter_next(iter);
}
#endif
diff --git a/revision.c b/revision.c
index f32fbc5e2e..f28cbe5de8 100644
--- a/revision.c
+++ b/revision.c
@@ -128,9 +128,10 @@ static void paths_and_oids_clear(struct hashmap *map)
{
struct hashmap_iter iter;
struct path_and_oids_entry *entry;
- hashmap_iter_init(map, &iter);
- while ((entry = (struct path_and_oids_entry *)hashmap_iter_next(&iter))) {
+ hashmap_for_each_entry(map, &iter, entry,
+ struct path_and_oids_entry,
+ ent /* member name */) {
oidset_clear(&entry->trees);
free(entry->path);
}
@@ -242,8 +243,9 @@ void mark_trees_uninteresting_sparse(struct repository *r,
add_children_by_path(r, tree, &map);
}
- hashmap_iter_init(&map, &map_iter);
- while ((entry = hashmap_iter_next(&map_iter)))
+ hashmap_for_each_entry(&map, &map_iter, entry,
+ struct path_and_oids_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 5463729ab8..5319933e1d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -99,8 +99,8 @@ static void submodule_cache_clear(struct submodule_cache *cache)
* allocation of struct submodule entries. Each is allocated by
* their .gitmodules blob sha1 and submodule name.
*/
- hashmap_iter_init(&cache->for_name, &iter);
- while ((entry = hashmap_iter_next(&iter)))
+ hashmap_for_each_entry(&cache->for_name, &iter, entry,
+ struct submodule_entry, ent /* member name */)
free_one_config(entry);
hashmap_free(&cache->for_path, 1);
@@ -556,7 +556,9 @@ static const struct submodule *config_from(struct submodule_cache *cache,
struct hashmap_iter iter;
struct submodule_entry *entry;
- entry = hashmap_iter_first(&cache->for_name, &iter);
+ entry = hashmap_iter_first_entry(&cache->for_name, &iter,
+ struct submodule_entry,
+ ent /* member name */);
if (!entry)
return NULL;
return entry->config;
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 56846da64c..4ec5e11556 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -222,10 +222,11 @@ int cmd__hashmap(int argc, const char **argv)
free(entry);
} else if (!strcmp("iterate", cmd)) {
-
struct hashmap_iter iter;
- hashmap_iter_init(&map, &iter);
- while ((entry = hashmap_iter_next(&iter)))
+
+ hashmap_for_each_entry(&map, &iter, entry,
+ struct test_entry,
+ ent /* member name */)
printf("%s %s\n", entry->key, get_value(entry));
} else if (!strcmp("size", cmd)) {
diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
index b99a37080d..9d4664d6a4 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -41,17 +41,13 @@ static void dump_run(void)
die("non-threaded code path used");
}
- dir = hashmap_iter_first(&the_index.dir_hash, &iter_dir);
- while (dir) {
+ hashmap_for_each_entry(&the_index.dir_hash, &iter_dir, dir,
+ struct dir_entry, ent /* member name */)
printf("dir %08x %7d %s\n", dir->ent.hash, dir->nr, dir->name);
- dir = hashmap_iter_next(&iter_dir);
- }
- ce = hashmap_iter_first(&the_index.name_hash, &iter_cache);
- while (ce) {
+ hashmap_for_each_entry(&the_index.name_hash, &iter_cache, ce,
+ struct cache_entry, ent /* member name */)
printf("name %08x %s\n", ce->ent.hash, ce->name);
- ce = hashmap_iter_next(&iter_cache);
- }
discard_cache();
}
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 16/20] hashmap: hashmap_{put,remove} return hashmap_entry *
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (14 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 15/20] hashmap: use *_entry APIs for iteration Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 17/20] hashmap: introduce hashmap_free_entries Eric Wong
` (6 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
And add *_entry variants to perform container_of as necessary
to simplify most callers.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
hashmap.c | 8 +++++---
hashmap.h | 15 ++++++++++++---
range-diff.c | 4 +++-
remote.c | 3 ++-
submodule-config.c | 4 +++-
t/helper/test-hashmap.c | 9 +++++++--
6 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/hashmap.c b/hashmap.c
index deb5fdf28c..1b60f97cf2 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -219,8 +219,9 @@ void hashmap_add(struct hashmap *map, struct hashmap_entry *entry)
}
}
-void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata)
+struct hashmap_entry *hashmap_remove(struct hashmap *map,
+ const struct hashmap_entry *key,
+ const void *keydata)
{
struct hashmap_entry *old;
struct hashmap_entry **e = find_entry_ptr(map, key, keydata);
@@ -242,7 +243,8 @@ void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
return old;
}
-void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry)
+struct hashmap_entry *hashmap_put(struct hashmap *map,
+ struct hashmap_entry *entry)
{
struct hashmap_entry *old = hashmap_remove(map, entry, NULL);
hashmap_add(map, entry);
diff --git a/hashmap.h b/hashmap.h
index 8d4b3907b4..bc3b10e097 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -349,7 +349,11 @@ void hashmap_add(struct hashmap *map, struct hashmap_entry *entry);
* `entry` is the entry to add or replace.
* Returns the replaced entry, or NULL if not found (i.e. the entry was added).
*/
-void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
+struct hashmap_entry *hashmap_put(struct hashmap *map,
+ struct hashmap_entry *entry);
+
+#define hashmap_put_entry(map, keyvar, type, member) \
+ container_of_or_null(hashmap_put(map, &(keyvar)->member), type, member)
/*
* Removes a hashmap entry matching the specified key. If the hashmap contains
@@ -358,8 +362,13 @@ void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
*
* Argument explanation is the same as in `hashmap_get`.
*/
-void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
- const void *keydata);
+struct hashmap_entry *hashmap_remove(struct hashmap *map,
+ const struct hashmap_entry *key,
+ const void *keydata);
+
+#define hashmap_remove_entry(map, keyvar, keydata, type, member) \
+ container_of_or_null(hashmap_remove(map, &(keyvar)->member, keydata), \
+ type, member)
/*
* Returns the `bucket` an entry is stored in.
diff --git a/range-diff.c b/range-diff.c
index c51cfd5556..e5e7820bfe 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -229,7 +229,9 @@ static void find_exact_matches(struct string_list *a, struct string_list *b)
util->patch = b->items[i].string;
util->diff = util->patch + util->diff_offset;
hashmap_entry_init(&util->e, strhash(util->diff));
- other = hashmap_remove(&map, &util->e, NULL);
+ other = hashmap_remove_entry(&map, util, NULL,
+ struct patch_util,
+ e /* member name */);
if (other) {
if (other->matching >= 0)
BUG("already assigned!");
diff --git a/remote.c b/remote.c
index fa9cadcfbd..5fcddcd88d 100644
--- a/remote.c
+++ b/remote.c
@@ -162,7 +162,8 @@ static struct remote *make_remote(const char *name, int len)
remotes[remotes_nr++] = ret;
hashmap_entry_init(&ret->ent, lookup_entry.hash);
- replaced = hashmap_put(&remotes_hash, &ret->ent);
+ replaced = hashmap_put_entry(&remotes_hash, ret, struct remote,
+ ent /* member name */);
assert(replaced == NULL); /* no previous entry overwritten */
return ret;
}
diff --git a/submodule-config.c b/submodule-config.c
index 5319933e1d..a289d195f6 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -141,7 +141,9 @@ static void cache_remove_path(struct submodule_cache *cache,
struct submodule_entry *removed;
hashmap_entry_init(&e.ent, hash);
e.config = submodule;
- removed = hashmap_remove(&cache->for_path, &e.ent, NULL);
+ removed = hashmap_remove_entry(&cache->for_path, &e, NULL,
+ struct submodule_entry,
+ ent /* member name */);
free(removed);
}
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 4ec5e11556..07a93a2aec 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -189,7 +189,9 @@ int cmd__hashmap(int argc, const char **argv)
entry = alloc_test_entry(hash, p1, p2);
/* add / replace entry */
- entry = hashmap_put(&map, &entry->ent);
+ entry = hashmap_put_entry(&map, entry,
+ struct test_entry,
+ ent /* member name */);
/* print and free replaced entry, if any */
puts(entry ? get_value(entry) : "NULL");
@@ -212,10 +214,13 @@ int cmd__hashmap(int argc, const char **argv)
/* setup static key */
struct hashmap_entry key;
+ struct hashmap_entry *rm;
hashmap_entry_init(&key, hash);
/* remove entry from hashmap */
- entry = hashmap_remove(&map, &key, p1);
+ rm = hashmap_remove(&map, &key, p1);
+ entry = rm ? container_of(rm, struct test_entry, ent)
+ : NULL;
/* print result and free entry*/
puts(entry ? get_value(entry) : "NULL");
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 17/20] hashmap: introduce hashmap_free_entries
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (15 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 16/20] hashmap: hashmap_{put,remove} return hashmap_entry * Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 18/20] OFFSETOF_VAR macro to simplify hashmap iterators Eric Wong
` (5 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
`hashmap_free_entries' behaves like `container_of' and passes
the offset of the hashmap_entry struct to the internal
`hashmap_free_' function, allowing the function to free any
struct pointer regardless of where the hashmap_entry field
is located.
`hashmap_free' no longer takes any arguments aside from
the hashmap itself.
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
blame.c | 2 +-
builtin/fetch.c | 6 +++---
config.c | 2 +-
diff.c | 6 ++++--
diffcore-rename.c | 2 +-
hashmap.c | 11 ++++++++---
hashmap.h | 19 +++++++++++++------
merge-recursive.c | 7 ++++---
name-hash.c | 4 ++--
oidmap.c | 4 +++-
patch-ids.c | 2 +-
range-diff.c | 2 +-
ref-filter.c | 3 ++-
revision.c | 2 +-
sequencer.c | 4 ++--
submodule-config.c | 4 ++--
t/helper/test-hashmap.c | 6 +++---
17 files changed, 52 insertions(+), 34 deletions(-)
diff --git a/blame.c b/blame.c
index 3d8accf902..f33af0da9f 100644
--- a/blame.c
+++ b/blame.c
@@ -433,7 +433,7 @@ static void get_fingerprint(struct fingerprint *result,
static void free_fingerprint(struct fingerprint *f)
{
- hashmap_free(&f->map, 0);
+ hashmap_free(&f->map);
free(f->entries);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 476c2416e3..09f7170616 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -366,7 +366,7 @@ static void find_non_local_tags(const struct ref *refs,
item = refname_hash_add(&remote_refs, ref->name, &ref->old_oid);
string_list_insert(&remote_refs_list, ref->name);
}
- hashmap_free(&existing_refs, 1);
+ hashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);
/*
* We may have a final lightweight tag that needs to be
@@ -401,7 +401,7 @@ static void find_non_local_tags(const struct ref *refs,
**tail = rm;
*tail = &rm->next;
}
- hashmap_free(&remote_refs, 1);
+ hashmap_free_entries(&remote_refs, struct refname_hash_entry, ent);
string_list_clear(&remote_refs_list, 0);
}
@@ -530,7 +530,7 @@ static struct ref *get_ref_map(struct remote *remote,
}
}
}
- hashmap_free(&existing_refs, 1);
+ hashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);
return ref_map;
}
diff --git a/config.c b/config.c
index 8433f74371..4d05dbc15a 100644
--- a/config.c
+++ b/config.c
@@ -1948,7 +1948,7 @@ void git_configset_clear(struct config_set *cs)
free(entry->key);
string_list_clear(&entry->value_list, 1);
}
- hashmap_free(&cs->config_hash, 1);
+ hashmap_free_entries(&cs->config_hash, struct config_set_element, ent);
cs->hash_initialized = 0;
free(cs->list.items);
cs->list.nr = 0;
diff --git a/diff.c b/diff.c
index 5eaf689fcc..f94d9f96af 100644
--- a/diff.c
+++ b/diff.c
@@ -6236,8 +6236,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
if (o->color_moved == COLOR_MOVED_ZEBRA_DIM)
dim_moved_lines(o);
- hashmap_free(&add_lines, 1);
- hashmap_free(&del_lines, 1);
+ hashmap_free_entries(&add_lines, struct moved_entry,
+ ent);
+ hashmap_free_entries(&del_lines, struct moved_entry,
+ ent);
}
for (i = 0; i < esm.nr; i++)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 611b08f463..994609ed58 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -358,7 +358,7 @@ static int find_exact_renames(struct diff_options *options)
renames += find_identical_files(&file_table, i, options);
/* Free the hash data structure and entries */
- hashmap_free(&file_table, 1);
+ hashmap_free_entries(&file_table, struct file_similarity, entry);
return renames;
}
diff --git a/hashmap.c b/hashmap.c
index 1b60f97cf2..65b447f6cd 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -171,16 +171,21 @@ void hashmap_init(struct hashmap *map, hashmap_cmp_fn equals_function,
map->do_count_items = 1;
}
-void hashmap_free(struct hashmap *map, int free_entries)
+void hashmap_free_(struct hashmap *map, ssize_t entry_offset)
{
if (!map || !map->table)
return;
- if (free_entries) {
+ if (entry_offset >= 0) { /* called by hashmap_free_entries */
struct hashmap_iter iter;
struct hashmap_entry *e;
+
hashmap_iter_init(map, &iter);
while ((e = hashmap_iter_next(&iter)))
- free(e);
+ /*
+ * like container_of, but using caller-calculated
+ * offset (caller being hashmap_free_entries)
+ */
+ free((char *)e - entry_offset);
}
free(map->table);
memset(map, 0, sizeof(*map));
diff --git a/hashmap.h b/hashmap.h
index bc3b10e097..171d6ddb76 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -96,7 +96,7 @@
* }
*
* if (!strcmp("end", action)) {
- * hashmap_free(&map, 1);
+ * hashmap_free_entries(&map, struct long2string, ent);
* break;
* }
* }
@@ -232,13 +232,20 @@ void hashmap_init(struct hashmap *map,
const void *equals_function_data,
size_t initial_size);
+/* internal function for freeing hashmap */
+void hashmap_free_(struct hashmap *map, ssize_t offset);
+
/*
- * Frees a hashmap structure and allocated memory.
- *
- * If `free_entries` is true, each hashmap_entry in the map is freed as well
- * using stdlibs free().
+ * Frees a hashmap structure and allocated memory, leaves entries undisturbed
+ */
+#define hashmap_free(map) hashmap_free_(map, -1)
+
+/*
+ * Frees @map and all entries. @type is the struct type of the entry
+ * where @member is the hashmap_entry struct used to associate with @map
*/
-void hashmap_free(struct hashmap *map, int free_entries);
+#define hashmap_free_entries(map, type, member) \
+ hashmap_free_(map, offsetof(type, member));
/* hashmap_entry functions */
diff --git a/merge-recursive.c b/merge-recursive.c
index 73c7750448..34b3d54154 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2633,7 +2633,7 @@ static struct string_list *get_renames(struct merge_options *opt,
free(e->target_file);
string_list_clear(&e->source_files, 0);
}
- hashmap_free(&collisions, 1);
+ hashmap_free_entries(&collisions, struct collision_entry, ent);
return renames;
}
@@ -2853,7 +2853,7 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs,
strbuf_release(&e->new_dir);
/* possible_new_dirs already cleared in get_directory_renames */
}
- hashmap_free(dir_renames, 1);
+ hashmap_free_entries(dir_renames, struct dir_rename_entry, ent);
free(dir_renames);
free(pairs->queue);
@@ -3482,7 +3482,8 @@ int merge_trees(struct merge_options *opt,
string_list_clear(entries, 1);
free(entries);
- hashmap_free(&opt->current_file_dir_set, 1);
+ hashmap_free_entries(&opt->current_file_dir_set,
+ struct path_hashmap_entry, e);
if (clean < 0) {
unpack_trees_finish(opt);
diff --git a/name-hash.c b/name-hash.c
index 85a1ce982c..c86fe0f1df 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -728,6 +728,6 @@ void free_name_hash(struct index_state *istate)
return;
istate->name_hash_initialized = 0;
- hashmap_free(&istate->name_hash, 0);
- hashmap_free(&istate->dir_hash, 1);
+ hashmap_free(&istate->name_hash);
+ hashmap_free_entries(&istate->dir_hash, struct dir_entry, ent);
}
diff --git a/oidmap.c b/oidmap.c
index 4942599391..423aa014a3 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -25,7 +25,9 @@ void oidmap_free(struct oidmap *map, int free_entries)
{
if (!map)
return;
- hashmap_free(&map->map, free_entries);
+
+ /* TODO: make oidmap itself not depend on struct layouts */
+ hashmap_free_(&map->map, free_entries ? 0 : -1);
}
void *oidmap_get(const struct oidmap *map, const struct object_id *key)
diff --git a/patch-ids.c b/patch-ids.c
index 75f8c9f1a1..af17828e33 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -71,7 +71,7 @@ int init_patch_ids(struct repository *r, struct patch_ids *ids)
int free_patch_ids(struct patch_ids *ids)
{
- hashmap_free(&ids->patches, 1);
+ hashmap_free_entries(&ids->patches, struct patch_id, ent);
return 0;
}
diff --git a/range-diff.c b/range-diff.c
index e5e7820bfe..9df53569bb 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -241,7 +241,7 @@ static void find_exact_matches(struct string_list *a, struct string_list *b)
}
}
- hashmap_free(&map, 0);
+ hashmap_free(&map);
}
static void diffsize_consume(void *data, char *line, unsigned long len)
diff --git a/ref-filter.c b/ref-filter.c
index 4613df8826..0950b789e3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2172,7 +2172,8 @@ void ref_array_clear(struct ref_array *array)
used_atom_cnt = 0;
if (ref_to_worktree_map.worktrees) {
- hashmap_free(&(ref_to_worktree_map.map), 1);
+ hashmap_free_entries(&(ref_to_worktree_map.map),
+ struct ref_to_worktree_entry, ent);
free_worktrees(ref_to_worktree_map.worktrees);
ref_to_worktree_map.worktrees = NULL;
}
diff --git a/revision.c b/revision.c
index f28cbe5de8..8a5f866ae6 100644
--- a/revision.c
+++ b/revision.c
@@ -136,7 +136,7 @@ static void paths_and_oids_clear(struct hashmap *map)
free(entry->path);
}
- hashmap_free(map, 1);
+ hashmap_free_entries(map, struct path_and_oids_entry, ent);
}
static void paths_and_oids_insert(struct hashmap *map,
diff --git a/sequencer.c b/sequencer.c
index b3e7319b55..694b463518 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4772,7 +4772,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
oidmap_free(&commit2todo, 1);
oidmap_free(&state.commit2label, 1);
- hashmap_free(&state.labels, 1);
+ hashmap_free_entries(&state.labels, struct labels_entry, entry);
strbuf_release(&state.buf);
return 0;
@@ -5301,7 +5301,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
for (i = 0; i < todo_list->nr; i++)
free(subjects[i]);
free(subjects);
- hashmap_free(&subject2item, 1);
+ hashmap_free_entries(&subject2item, struct subject2item_entry, entry);
clear_commit_todo_item(&commit_todo);
diff --git a/submodule-config.c b/submodule-config.c
index a289d195f6..5462acc8ec 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -103,8 +103,8 @@ static void submodule_cache_clear(struct submodule_cache *cache)
struct submodule_entry, ent /* member name */)
free_one_config(entry);
- hashmap_free(&cache->for_path, 1);
- hashmap_free(&cache->for_name, 1);
+ hashmap_free_entries(&cache->for_path, struct submodule_entry, ent);
+ hashmap_free_entries(&cache->for_name, struct submodule_entry, ent);
cache->initialized = 0;
cache->gitmodules_read = 0;
}
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 07a93a2aec..6f2530dcc8 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -109,7 +109,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
hashmap_add(&map, &entries[i]->ent);
}
- hashmap_free(&map, 0);
+ hashmap_free(&map);
}
} else {
/* test map lookups */
@@ -129,7 +129,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
}
}
- hashmap_free(&map, 0);
+ hashmap_free(&map);
}
}
@@ -266,6 +266,6 @@ int cmd__hashmap(int argc, const char **argv)
}
strbuf_release(&line);
- hashmap_free(&map, 1);
+ hashmap_free_entries(&map, struct test_entry, ent);
return 0;
}
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 18/20] OFFSETOF_VAR macro to simplify hashmap iterators
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (16 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 17/20] hashmap: introduce hashmap_free_entries Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 19/20] hashmap: remove type arg from hashmap_{get,put,remove}_entry Eric Wong
` (4 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
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 for compilers without `__typeof__'.
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__'.
v3: use `__typeof__' to avoid clang warnings
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
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 | 13 +++++++++
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, 56 insertions(+), 45 deletions(-)
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 4a23b9090b..8605cb4202 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1337,4 +1337,17 @@ 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 a a variable of type which
+ * contains @member, instead of a specified type.
+ * @ptr is subject to multiple evaluation since we can't rely on __typeof__
+ * everywhere.
+ */
+#if defined(__GNUC__) /* clang sets this, too */
+#define OFFSETOF_VAR(ptr, member) offsetof(__typeof__(*ptr), member)
+#else /* !__GNUC__ */
+#define OFFSETOF_VAR(ptr, member) \
+ ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))
+#endif /* !__GNUC__ */
+
#endif
diff --git a/hashmap.h b/hashmap.h
index 171d6ddb76..96786c724a 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();
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 19/20] hashmap: remove type arg from hashmap_{get,put,remove}_entry
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (17 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 18/20] OFFSETOF_VAR macro to simplify hashmap iterators Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-06 23:30 ` [PATCH v3 20/20] hashmap_entry: remove first member requirement from docs Eric Wong
` (3 subsequent siblings)
22 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Since these macros already take a `keyvar' pointer of a known type,
we can rely on OFFSETOF_VAR to get the correct offset without
relying on non-portable `__typeof__' and `offsetof'.
Argument order is also rearranged, so `keyvar' and `member' are
sequential as they are used as: `keyvar->member'
Signed-off-by: Eric Wong <e@80x24.org>
Reviewed-by: Derrick Stolee <stolee@gmail.com>
---
attr.c | 2 +-
blame.c | 10 ++++-----
builtin/difftool.c | 2 +-
builtin/fast-export.c | 2 +-
config.c | 3 +--
diff.c | 6 ++----
hashmap.c | 2 +-
hashmap.h | 45 ++++++++++++++++++++++++++++++-----------
merge-recursive.c | 6 ++----
name-hash.c | 3 +--
patch-ids.c | 3 +--
range-diff.c | 4 +---
remote.c | 3 +--
revision.c | 3 +--
sub-process.c | 3 +--
submodule-config.c | 10 +++------
t/helper/test-hashmap.c | 4 +---
17 files changed, 56 insertions(+), 55 deletions(-)
diff --git a/attr.c b/attr.c
index 9849106627..15f0efdf60 100644
--- a/attr.c
+++ b/attr.c
@@ -103,7 +103,7 @@ static void *attr_hashmap_get(struct attr_hashmap *map,
hashmap_entry_init(&k.ent, memhash(key, keylen));
k.key = key;
k.keylen = keylen;
- e = hashmap_get_entry(&map->map, &k, NULL, struct attr_hash_entry, ent);
+ e = hashmap_get_entry(&map->map, &k, ent, NULL);
return e ? e->value : NULL;
}
diff --git a/blame.c b/blame.c
index 90b247abf9..6384f48133 100644
--- a/blame.c
+++ b/blame.c
@@ -419,8 +419,8 @@ static void get_fingerprint(struct fingerprint *result,
continue;
hashmap_entry_init(&entry->entry, hash);
- found_entry = hashmap_get_entry(&result->map, entry, NULL,
- struct fingerprint_entry, entry);
+ found_entry = hashmap_get_entry(&result->map, entry,
+ /* member name */ entry, NULL);
if (found_entry) {
found_entry->count += 1;
} else {
@@ -452,8 +452,7 @@ static int fingerprint_similarity(struct fingerprint *a, struct fingerprint *b)
hashmap_for_each_entry(&b->map, &iter, entry_b,
entry /* member name */) {
- entry_a = hashmap_get_entry(&a->map, entry_b, NULL,
- struct fingerprint_entry, entry);
+ entry_a = hashmap_get_entry(&a->map, entry_b, entry, NULL);
if (entry_a) {
intersection += entry_a->count < entry_b->count ?
entry_a->count : entry_b->count;
@@ -474,8 +473,7 @@ static void fingerprint_subtract(struct fingerprint *a, struct fingerprint *b)
hashmap_for_each_entry(&b->map, &iter, entry_b,
entry /* member name */) {
- entry_a = hashmap_get_entry(&a->map, entry_b, NULL,
- struct fingerprint_entry, entry);
+ entry_a = hashmap_get_entry(&a->map, entry_b, entry, NULL);
if (entry_a) {
if (entry_a->count <= entry_b->count)
hashmap_remove(&a->map, &entry_b->entry, NULL);
diff --git a/builtin/difftool.c b/builtin/difftool.c
index f2d4d1e0f8..c280e682b2 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -167,7 +167,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
FLEX_ALLOC_STR(e, path, path);
hashmap_entry_init(&e->entry, strhash(path));
- existing = hashmap_get_entry(map, e, NULL, struct pair_entry, entry);
+ existing = hashmap_get_entry(map, e, entry, NULL);
if (existing) {
free(e);
e = existing;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index ef0578bf90..e3de403efd 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -156,7 +156,7 @@ static const void *anonymize_mem(struct hashmap *map,
hashmap_entry_init(&key.hash, memhash(orig, *len));
key.orig = orig;
key.orig_len = *len;
- ret = hashmap_get_entry(map, &key, NULL, struct anonymized_entry, hash);
+ ret = hashmap_get_entry(map, &key, hash, NULL);
if (!ret) {
ret = xmalloc(sizeof(*ret));
diff --git a/config.c b/config.c
index 77ed00bfbf..a4fa464ed2 100644
--- a/config.c
+++ b/config.c
@@ -1863,8 +1863,7 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
hashmap_entry_init(&k.ent, strhash(normalized_key));
k.key = normalized_key;
- found_entry = hashmap_get_entry(&cs->config_hash, &k, NULL,
- struct config_set_element, ent);
+ found_entry = hashmap_get_entry(&cs->config_hash, &k, ent, NULL);
free(normalized_key);
return found_entry;
}
diff --git a/diff.c b/diff.c
index 051de9832d..a9ecb93af3 100644
--- a/diff.c
+++ b/diff.c
@@ -1146,15 +1146,13 @@ 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_entry(hm, key, NULL,
- struct moved_entry, ent);
+ match = hashmap_get_entry(hm, key, ent, NULL);
free(key);
break;
case DIFF_SYMBOL_MINUS:
hm = add_lines;
key = prepare_entry(o, n);
- match = hashmap_get_entry(hm, key, NULL,
- struct moved_entry, ent);
+ match = hashmap_get_entry(hm, key, ent, NULL);
free(key);
break;
default:
diff --git a/hashmap.c b/hashmap.c
index 65b447f6cd..39c13110bc 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -311,7 +311,7 @@ const void *memintern(const void *data, size_t len)
/* lookup interned string in pool */
hashmap_entry_init(&key.ent, memhash(data, len));
key.len = len;
- e = hashmap_get_entry(&map, &key, data, struct pool_entry, ent);
+ e = hashmap_get_entry(&map, &key, ent, data);
if (!e) {
/* not found: create it */
FLEX_ALLOC_MEM(e, data, data, len);
diff --git a/hashmap.h b/hashmap.h
index 96786c724a..bf4a05937d 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -63,7 +63,7 @@
* k.key = key;
*
* flags &= ~COMPARE_VALUE;
- * e = hashmap_get_entry(&map, &k, NULL, struct long2string, ent);
+ * e = hashmap_get_entry(&map, &k, ent, NULL);
* if (e) {
* printf("first: %ld %s\n", e->key, e->value);
* while ((e = hashmap_get_next_entry(&map, e,
@@ -359,8 +359,17 @@ void hashmap_add(struct hashmap *map, struct hashmap_entry *entry);
struct hashmap_entry *hashmap_put(struct hashmap *map,
struct hashmap_entry *entry);
-#define hashmap_put_entry(map, keyvar, type, member) \
- container_of_or_null(hashmap_put(map, &(keyvar)->member), type, member)
+/*
+ * Adds or replaces a hashmap entry contained within @keyvar,
+ * where @keyvar is a pointer to a struct containing a
+ * "struct hashmap_entry" @member.
+ *
+ * Returns the replaced pointer which is of the same type as @keyvar,
+ * or NULL if not found.
+ */
+#define hashmap_put_entry(map, keyvar, member) \
+ container_of_or_null_offset(hashmap_put(map, &(keyvar)->member), \
+ OFFSETOF_VAR(keyvar, member))
/*
* Removes a hashmap entry matching the specified key. If the hashmap contains
@@ -373,9 +382,20 @@ struct hashmap_entry *hashmap_remove(struct hashmap *map,
const struct hashmap_entry *key,
const void *keydata);
-#define hashmap_remove_entry(map, keyvar, keydata, type, member) \
- container_of_or_null(hashmap_remove(map, &(keyvar)->member, keydata), \
- type, member)
+/*
+ * Removes a hashmap entry contained within @keyvar,
+ * where @keyvar is a pointer to a struct containing a
+ * "struct hashmap_entry" @member.
+ *
+ * See `hashmap_get` for an explanation of @keydata
+ *
+ * Returns the replaced pointer which is of the same type as @keyvar,
+ * or NULL if not found.
+ */
+#define hashmap_remove_entry(map, keyvar, member, keydata) \
+ container_of_or_null_offset( \
+ hashmap_remove(map, &(keyvar)->member, keydata), \
+ OFFSETOF_VAR(keyvar, member))
/*
* Returns the `bucket` an entry is stored in.
@@ -436,13 +456,14 @@ static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map,
OFFSETOF_VAR(var, member)))
/*
- * returns a @pointer of @type matching @keyvar, or NULL if nothing found.
- * @keyvar is a pointer of @type
- * @member is the name of the "struct hashmap_entry" field in @type
+ * returns a pointer of type matching @keyvar, or NULL if nothing found.
+ * @keyvar is a pointer to a struct containing a
+ * "struct hashmap_entry" @member.
*/
-#define hashmap_get_entry(map, keyvar, keydata, type, member) \
- container_of_or_null(hashmap_get(map, &(keyvar)->member, keydata), \
- type, member)
+#define hashmap_get_entry(map, keyvar, member, keydata) \
+ container_of_or_null_offset( \
+ hashmap_get(map, &(keyvar)->member, keydata), \
+ OFFSETOF_VAR(keyvar, member))
#define hashmap_get_entry_from_hash(map, hash, keydata, type, member) \
container_of_or_null(hashmap_get_from_hash(map, hash, keydata), \
diff --git a/merge-recursive.c b/merge-recursive.c
index 3abba3a618..8787a40b0c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -65,8 +65,7 @@ static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
return NULL;
hashmap_entry_init(&key.ent, strhash(dir));
key.dir = dir;
- return hashmap_get_entry(hashmap, &key, NULL,
- struct dir_rename_entry, ent);
+ return hashmap_get_entry(hashmap, &key, ent, NULL);
}
static int dir_rename_cmp(const void *unused_cmp_data,
@@ -104,8 +103,7 @@ static struct collision_entry *collision_find_entry(struct hashmap *hashmap,
hashmap_entry_init(&key.ent, strhash(target_file));
key.target_file = target_file;
- return hashmap_get_entry(hashmap, &key, NULL,
- struct collision_entry, ent);
+ return hashmap_get_entry(hashmap, &key, ent, NULL);
}
static int collision_cmp(const void *unused_cmp_data,
diff --git a/name-hash.c b/name-hash.c
index 3cda22b657..ceb1d7bd6f 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -37,8 +37,7 @@ static struct dir_entry *find_dir_entry__hash(struct index_state *istate,
struct dir_entry key;
hashmap_entry_init(&key.ent, hash);
key.namelen = namelen;
- return hashmap_get_entry(&istate->dir_hash, &key, name,
- struct dir_entry, ent);
+ return hashmap_get_entry(&istate->dir_hash, &key, ent, name);
}
static struct dir_entry *find_dir_entry(struct index_state *istate,
diff --git a/patch-ids.c b/patch-ids.c
index af17828e33..12aa6d494b 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -101,8 +101,7 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
if (init_patch_id_entry(&patch, commit, ids))
return NULL;
- return hashmap_get_entry(&ids->patches, &patch, NULL,
- struct patch_id, ent);
+ return hashmap_get_entry(&ids->patches, &patch, ent, NULL);
}
struct patch_id *add_commit_patch_id(struct commit *commit,
diff --git a/range-diff.c b/range-diff.c
index 9df53569bb..22ad959cee 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -229,9 +229,7 @@ static void find_exact_matches(struct string_list *a, struct string_list *b)
util->patch = b->items[i].string;
util->diff = util->patch + util->diff_offset;
hashmap_entry_init(&util->e, strhash(util->diff));
- other = hashmap_remove_entry(&map, util, NULL,
- struct patch_util,
- e /* member name */);
+ other = hashmap_remove_entry(&map, util, e, NULL);
if (other) {
if (other->matching >= 0)
BUG("already assigned!");
diff --git a/remote.c b/remote.c
index 5fcddcd88d..5c4666b53a 100644
--- a/remote.c
+++ b/remote.c
@@ -162,8 +162,7 @@ static struct remote *make_remote(const char *name, int len)
remotes[remotes_nr++] = ret;
hashmap_entry_init(&ret->ent, lookup_entry.hash);
- replaced = hashmap_put_entry(&remotes_hash, ret, struct remote,
- ent /* member name */);
+ replaced = hashmap_put_entry(&remotes_hash, ret, ent);
assert(replaced == NULL); /* no previous entry overwritten */
return ret;
}
diff --git a/revision.c b/revision.c
index 5abd4a1fe7..6688f89d0d 100644
--- a/revision.c
+++ b/revision.c
@@ -151,8 +151,7 @@ static void paths_and_oids_insert(struct hashmap *map,
key.path = (char *)path;
oidset_init(&key.trees, 0);
- entry = hashmap_get_entry(map, &key, NULL,
- struct path_and_oids_entry, ent);
+ entry = hashmap_get_entry(map, &key, ent, NULL);
if (!entry) {
entry = xcalloc(1, sizeof(struct path_and_oids_entry));
hashmap_entry_init(&entry->ent, hash);
diff --git a/sub-process.c b/sub-process.c
index ad94f72665..1b1af9dcbd 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -24,8 +24,7 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch
hashmap_entry_init(&key.ent, strhash(cmd));
key.cmd = cmd;
- return hashmap_get_entry(hashmap, &key, NULL,
- struct subprocess_entry, ent);
+ return hashmap_get_entry(hashmap, &key, ent, NULL);
}
int subprocess_read_status(int fd, struct strbuf *status)
diff --git a/submodule-config.c b/submodule-config.c
index c22855cd38..401a9b2382 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -141,9 +141,7 @@ static void cache_remove_path(struct submodule_cache *cache,
struct submodule_entry *removed;
hashmap_entry_init(&e.ent, hash);
e.config = submodule;
- removed = hashmap_remove_entry(&cache->for_path, &e, NULL,
- struct submodule_entry,
- ent /* member name */);
+ removed = hashmap_remove_entry(&cache->for_path, &e, ent, NULL);
free(removed);
}
@@ -172,8 +170,7 @@ static const struct submodule *cache_lookup_path(struct submodule_cache *cache,
hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
- entry = hashmap_get_entry(&cache->for_path, &key, NULL,
- struct submodule_entry, ent);
+ entry = hashmap_get_entry(&cache->for_path, &key, ent, NULL);
if (entry)
return entry->config;
return NULL;
@@ -193,8 +190,7 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
hashmap_entry_init(&key.ent, hash);
key.config = &key_config;
- entry = hashmap_get_entry(&cache->for_name, &key, NULL,
- struct submodule_entry, ent);
+ entry = hashmap_get_entry(&cache->for_name, &key, ent, NULL);
if (entry)
return entry->config;
return NULL;
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index f89d1194ef..cc577c8956 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -189,9 +189,7 @@ int cmd__hashmap(int argc, const char **argv)
entry = alloc_test_entry(hash, p1, p2);
/* add / replace entry */
- entry = hashmap_put_entry(&map, entry,
- struct test_entry,
- ent /* member name */);
+ entry = hashmap_put_entry(&map, entry, ent);
/* print and free replaced entry, if any */
puts(entry ? get_value(entry) : "NULL");
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 20/20] hashmap_entry: remove first member requirement from docs
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (18 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 19/20] hashmap: remove type arg from hashmap_{get,put,remove}_entry Eric Wong
@ 2019-10-06 23:30 ` Eric Wong
2019-10-07 1:52 ` Junio C Hamano
2019-10-07 1:24 ` [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Junio C Hamano
` (2 subsequent siblings)
22 siblings, 1 reply; 30+ messages in thread
From: Eric Wong @ 2019-10-06 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Comments stating that "struct hashmap_entry" must be the first
member in a struct are no longer valid.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Eric Wong <e@80x24.org>
---
attr.c | 2 +-
builtin/fetch.c | 2 +-
hashmap.h | 4 ++--
merge-recursive.h | 4 ++--
ref-filter.c | 2 +-
refs.c | 2 +-
remote.h | 2 +-
sub-process.h | 2 +-
t/helper/test-hashmap.c | 1 +
9 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/attr.c b/attr.c
index 15f0efdf60..e5c951db69 100644
--- a/attr.c
+++ b/attr.c
@@ -62,7 +62,7 @@ static struct attr_hashmap g_attr_hashmap;
/* The container for objects stored in "struct attr_hashmap" */
struct attr_hash_entry {
- struct hashmap_entry ent; /* must be the first member! */
+ struct hashmap_entry ent;
const char *key; /* the key; memory should be owned by value */
size_t keylen; /* length of the key */
void *value; /* the stored value */
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 09f7170616..a3154a4edb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -251,7 +251,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1)
}
struct refname_hash_entry {
- struct hashmap_entry ent; /* must be the first member */
+ struct hashmap_entry ent;
struct object_id oid;
int ignore;
char refname[FLEX_ARRAY];
diff --git a/hashmap.h b/hashmap.h
index bf4a05937d..bd2701549f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -13,7 +13,7 @@
*
* struct hashmap map;
* struct long2string {
- * struct hashmap_entry ent; // must be the first member!
+ * struct hashmap_entry ent;
* long key;
* char value[FLEX_ARRAY]; // be careful with allocating on stack!
* };
@@ -141,7 +141,7 @@ static inline unsigned int oidhash(const struct object_id *oid)
/*
* struct hashmap_entry is an opaque structure representing an entry in the
- * hash table, which must be used as first member of user data structures.
+ * hash table.
* Ideally it should be followed by an int-sized member to prevent unused
* memory on 64-bit systems due to alignment.
*/
diff --git a/merge-recursive.h b/merge-recursive.h
index c2b7bb65c6..daa742568e 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -50,7 +50,7 @@ struct merge_options {
* in get_directory_renames() for details
*/
struct dir_rename_entry {
- struct hashmap_entry ent; /* must be the first member! */
+ struct hashmap_entry ent;
char *dir;
unsigned non_unique_new_dir:1;
struct strbuf new_dir;
@@ -58,7 +58,7 @@ struct dir_rename_entry {
};
struct collision_entry {
- struct hashmap_entry ent; /* must be the first member! */
+ struct hashmap_entry ent;
char *target_file;
struct string_list source_files;
unsigned reported_already:1;
diff --git a/ref-filter.c b/ref-filter.c
index 0950b789e3..5c10a343c6 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -79,7 +79,7 @@ static struct expand_data {
} oi, oi_deref;
struct ref_to_worktree_entry {
- struct hashmap_entry ent; /* must be the first member! */
+ struct hashmap_entry ent;
struct worktree *wt; /* key is wt->head_ref */
};
diff --git a/refs.c b/refs.c
index 2d3eb40f39..1ab0bb54d3 100644
--- a/refs.c
+++ b/refs.c
@@ -1772,7 +1772,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
struct ref_store_hash_entry
{
- struct hashmap_entry ent; /* must be the first member! */
+ struct hashmap_entry ent;
struct ref_store *refs;
diff --git a/remote.h b/remote.h
index 83e885672b..0e1d2b245b 100644
--- a/remote.h
+++ b/remote.h
@@ -14,7 +14,7 @@ enum {
};
struct remote {
- struct hashmap_entry ent; /* must be first */
+ struct hashmap_entry ent;
const char *name;
int origin, configured_in_repo;
diff --git a/sub-process.h b/sub-process.h
index 0d12708b8c..e85f21fa1a 100644
--- a/sub-process.h
+++ b/sub-process.h
@@ -24,7 +24,7 @@
/* Members should not be accessed directly. */
struct subprocess_entry {
- struct hashmap_entry ent; /* must be the first member! */
+ struct hashmap_entry ent;
const char *cmd;
struct child_process process;
};
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index cc577c8956..f38706216f 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -5,6 +5,7 @@
struct test_entry
{
+ int padding; /* hashmap entry no longer needs to be the first member */
struct hashmap_entry ent;
/* key and value as two \0-terminated strings */
char key[FLEX_ARRAY];
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (19 preceding siblings ...)
2019-10-06 23:30 ` [PATCH v3 20/20] hashmap_entry: remove first member requirement from docs Eric Wong
@ 2019-10-07 1:24 ` Junio C Hamano
2019-10-08 8:58 ` Johannes Schindelin
2019-10-08 13:59 ` Derrick Stolee
22 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2019-10-07 1:24 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Eric Wong <e@80x24.org> writes:
> v3 changes:
> - use __typeof__ to avoid invalid clang warning on uninitialized var
> - formatting fixes recommended by Stolee
> - add Reviewed-by for Stolee
> - add patch 20 to update docs to drop first member requirement
I just did
git checkout ew/hashmap
git checkout master...
git am -s <mbox-with-these-patches
git diff @{-1}
and all the differences between them I agree with (and needless to
say, I looked at all the changes in the previous round).
Looks much nicer.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 20/20] hashmap_entry: remove first member requirement from docs
2019-10-06 23:30 ` [PATCH v3 20/20] hashmap_entry: remove first member requirement from docs Eric Wong
@ 2019-10-07 1:52 ` Junio C Hamano
2019-10-07 8:43 ` [PATCH v3 21/20] convert: drop invalid comment for subprocess_entry Eric Wong
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2019-10-07 1:52 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Eric Wong <e@80x24.org> writes:
> Comments stating that "struct hashmap_entry" must be the first
> member in a struct are no longer valid.
>
> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
> attr.c | 2 +-
> builtin/fetch.c | 2 +-
> hashmap.h | 4 ++--
> merge-recursive.h | 4 ++--
> ref-filter.c | 2 +-
> refs.c | 2 +-
> remote.h | 2 +-
> sub-process.h | 2 +-
> t/helper/test-hashmap.c | 1 +
> 9 files changed, 11 insertions(+), 10 deletions(-)
After this patch, there is one "/* must be the first member! */"
comment left in convert.c, which is both misleading and unfortunate;
a structure 'subprocess_entry' wants to be the first field of any
enclosing structure, where 'subprocess_entry' has a hashmap_entry as
its first field.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 21/20] convert: drop invalid comment for subprocess_entry
2019-10-07 1:52 ` Junio C Hamano
@ 2019-10-07 8:43 ` Eric Wong
2019-10-08 2:15 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Eric Wong @ 2019-10-07 8:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
>
> > Comments stating that "struct hashmap_entry" must be the first
> > member in a struct are no longer valid.
>
> After this patch, there is one "/* must be the first member! */"
> comment left in convert.c, which is both misleading and unfortunate;
> a structure 'subprocess_entry' wants to be the first field of any
> enclosing structure, where 'subprocess_entry' has a hashmap_entry as
> its first field.
Oops, I was only grepping for hashmap_entry :x
Thanks for the sharp eyes :>
Also, the first member requirement for the oidmap stuff could be
lifted sometime in the future...
----8<-----
Subject: [PATCH] convert: drop invalid comment for subprocess_entry
"struct hashmap_entry" inside "struct subprocess_entry"
no longer needs to be the first member of any struct,
so the old comment is no longer true.
Signed-off-by: Eric Wong <e@80x24.org>
Reported-by: Junio C Hamano <gitster@pobox.com>
---
convert.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/convert.c b/convert.c
index 94ff837649..93c1e1eae9 100644
--- a/convert.c
+++ b/convert.c
@@ -753,7 +753,7 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le
#define CAP_DELAY (1u<<2)
struct cmd2process {
- struct subprocess_entry subprocess; /* must be the first member! */
+ struct subprocess_entry subprocess;
unsigned int supported_capabilities;
};
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 21/20] convert: drop invalid comment for subprocess_entry
2019-10-07 8:43 ` [PATCH v3 21/20] convert: drop invalid comment for subprocess_entry Eric Wong
@ 2019-10-08 2:15 ` Junio C Hamano
2019-10-08 2:56 ` Eric Wong
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2019-10-08 2:15 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Eric Wong <e@80x24.org> writes:
> "struct hashmap_entry" inside "struct subprocess_entry"
> no longer needs to be the first member of any struct,
> so the old comment is no longer true.
Hmm, is that true?
struct cmd2process {
struct subprocess_entry subprocess;
unsigned int supported_capabilities;
};
static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
{
static int versions[] = {2, 0};
static struct subprocess_capability capabilities[] = {
{ "clean", CAP_CLEAN },
{ "smudge", CAP_SMUDGE },
{ "delay", CAP_DELAY },
{ NULL, 0 }
};
struct cmd2process *entry = (struct cmd2process *)subprocess;
return subprocess_handshake(subprocess, "git-filter", versions, NULL,
capabilities,
&entry->supported_capabilities);
}
The cast "struct subprocess_entry *" to "struct cmd2process *" we
see here does require that the address of the subprocess field must
be the same as the address of the structure itself. So I'd have to
say that the comment still is true, but not for the reasons of what
is in "struct subprocess_entry".
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 21/20] convert: drop invalid comment for subprocess_entry
2019-10-08 2:15 ` Junio C Hamano
@ 2019-10-08 2:56 ` Eric Wong
0 siblings, 0 replies; 30+ messages in thread
From: Eric Wong @ 2019-10-08 2:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Johannes Schindelin, Phillip Wood
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
>
> > "struct hashmap_entry" inside "struct subprocess_entry"
> > no longer needs to be the first member of any struct,
> > so the old comment is no longer true.
>
> Hmm, is that true?
>
> struct cmd2process {
> struct subprocess_entry subprocess;
> unsigned int supported_capabilities;
> };
>
> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
> {
> static int versions[] = {2, 0};
> static struct subprocess_capability capabilities[] = {
> { "clean", CAP_CLEAN },
> { "smudge", CAP_SMUDGE },
> { "delay", CAP_DELAY },
> { NULL, 0 }
> };
> struct cmd2process *entry = (struct cmd2process *)subprocess;
> return subprocess_handshake(subprocess, "git-filter", versions, NULL,
> capabilities,
> &entry->supported_capabilities);
> }
>
> The cast "struct subprocess_entry *" to "struct cmd2process *" we
> see here does require that the address of the subprocess field must
> be the same as the address of the structure itself. So I'd have to
> say that the comment still is true, but not for the reasons of what
> is in "struct subprocess_entry".
Oops, right. Let's just leave it at 20 patches for this series
and skip this one, for now.
I seem to recall there's a bunch of places where we do casts
like that which could be rewritten more flexibly with container_of.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (20 preceding siblings ...)
2019-10-07 1:24 ` [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Junio C Hamano
@ 2019-10-08 8:58 ` Johannes Schindelin
2019-10-08 13:56 ` Derrick Stolee
2019-10-08 13:59 ` Derrick Stolee
22 siblings, 1 reply; 30+ messages in thread
From: Johannes Schindelin @ 2019-10-08 8:58 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, git, Derrick Stolee, Phillip Wood
Hi Eric & Junio,
On Sun, 6 Oct 2019, Eric Wong wrote:
> v3 changes:
> - use __typeof__ to avoid invalid clang warning on uninitialized var
> - formatting fixes recommended by Stolee
> - add Reviewed-by for Stolee
> - add patch 20 to update docs to drop first member requirement
This has quite a bit of fallout, even more than previous rounds, it
seems. I need at least these fixup???s to fix the build of `pu`:
https://github.com/git-for-windows/git/commit/f74259754971b427a14e6290681e18950824b99d
https://github.com/git-for-windows/git/commit/124c8bc08e974e76ca7d956dc07eb288e71d639e
https://github.com/git-for-windows/git/commit/45948433d1b48ff513fbd37f134c0f1491c78192
Junio, feel free to fetch and squash those into the appropriate merge
commit(s) (I feel that 4432e8b5f36 (Merge branch 'ds/sparse-cone' into
pu, 2019-10-08) may be the right spot).
Thanks,
Dscho
>
> v2 here:
> https://public-inbox.org/git/20190924010324.22619-1-e@80x24.org/
>
> The following changes since commit 745f6812895b31c02b29bdfe4ae8e5498f776c26:
>
> First batch after Git 2.23 (2019-08-22 12:41:04 -0700)
>
> are available in the Git repository at:
>
> https://80x24.org/git-svn.git hashmap-wip-v3
>
> for you to fetch changes up to 4ec5200646d6b9363e64e2002eb374aa888a6c88:
>
> hashmap_entry: remove first member requirement from docs (2019-10-06 23:05:22 +0000)
>
> ----------------------------------------------------------------
> Eric Wong (20):
> diff: use hashmap_entry_init on moved_entry.ent
> coccicheck: detect hashmap_entry.hash assignment
> packfile: use hashmap_entry in delta_base_cache_entry
> hashmap_entry_init takes "struct hashmap_entry *"
> hashmap_get_next takes "const struct hashmap_entry *"
> hashmap_add takes "struct hashmap_entry *"
> hashmap_get takes "const struct hashmap_entry *"
> hashmap_remove takes "const struct hashmap_entry *"
> hashmap_put takes "struct hashmap_entry *"
> introduce container_of macro
> hashmap_get_next returns "struct hashmap_entry *"
> hashmap: use *_entry APIs to wrap container_of
> hashmap_get{,_from_hash} return "struct hashmap_entry *"
> hashmap_cmp_fn takes hashmap_entry params
> hashmap: use *_entry APIs for iteration
> hashmap: hashmap_{put,remove} return hashmap_entry *
> hashmap: introduce hashmap_free_entries
> OFFSETOF_VAR macro to simplify hashmap iterators
> hashmap: remove type arg from hashmap_{get,put,remove}_entry
> hashmap_entry: remove first member requirement from docs
>
> attr.c | 24 ++---
> blame.c | 25 ++---
> builtin/describe.c | 21 +++--
> builtin/difftool.c | 56 +++++++-----
> builtin/fast-export.c | 15 ++-
> builtin/fetch.c | 32 ++++---
> config.c | 24 ++---
> contrib/coccinelle/hashmap.cocci | 16 ++++
> diff.c | 31 ++++---
> diffcore-rename.c | 15 ++-
> git-compat-util.h | 38 ++++++++
> hashmap.c | 58 +++++++-----
> hashmap.h | 176 +++++++++++++++++++++++++++++-------
> merge-recursive.c | 87 ++++++++++--------
> merge-recursive.h | 4 +-
> name-hash.c | 57 ++++++------
> oidmap.c | 20 ++--
> oidmap.h | 6 +-
> packfile.c | 22 +++--
> patch-ids.c | 18 ++--
> range-diff.c | 10 +-
> ref-filter.c | 33 ++++---
> refs.c | 25 +++--
> remote.c | 21 +++--
> remote.h | 2 +-
> revision.c | 28 +++---
> sequencer.c | 44 ++++++---
> sub-process.c | 20 ++--
> sub-process.h | 6 +-
> submodule-config.c | 52 ++++++-----
> t/helper/test-hashmap.c | 50 +++++-----
> t/helper/test-lazy-init-name-hash.c | 12 +--
> 32 files changed, 667 insertions(+), 381 deletions(-)
> create mode 100644 contrib/coccinelle/hashmap.cocci
>
> Range-diff against v2:
> 1: 51ba91d2f9 ! 1: 0f530552de diff: use hashmap_entry_init on moved_entry.ent
> @@ -10,6 +10,7 @@
> hashmap_add callers to take "struct hashmap_entry *".
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/diff.c b/diff.c
> --- a/diff.c
> 2: be4071d33d ! 2: d12ddb4327 coccicheck: detect hashmap_entry.hash assignment
> @@ -8,6 +8,7 @@
> hashmap_entry_init, instead.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/contrib/coccinelle/hashmap.cocci b/contrib/coccinelle/hashmap.cocci
> new file mode 100644
> 3: ff40a39bad ! 3: 624b89fbb4 packfile: use hashmap_entry in delta_base_cache_entry
> @@ -15,6 +15,7 @@
> systems.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/packfile.c b/packfile.c
> --- a/packfile.c
> 4: d1db0f6472 ! 4: 7512f2f2f7 hashmap_entry_init takes "struct hashmap_entry *"
> @@ -8,6 +8,7 @@
> safety and readability.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> @@ -212,8 +213,8 @@
> * and if it is on stack, you can just let it go out of scope).
> */
> -static inline void hashmap_entry_init(void *entry, unsigned int hash)
> -+static inline void
> -+hashmap_entry_init(struct hashmap_entry *e, unsigned int hash)
> ++static inline void hashmap_entry_init(struct hashmap_entry *e,
> ++ unsigned int hash)
> {
> - struct hashmap_entry *e = entry;
> e->hash = hash;
> 5: a293445e97 ! 5: dd167a1113 hashmap_get_next takes "const struct hashmap_entry *"
> @@ -6,6 +6,7 @@
> now detects invalid types being passed.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/diff.c b/diff.c
> --- a/diff.c
> 6: beabdab6fe ! 6: 53f429ef6f hashmap_add takes "struct hashmap_entry *"
> @@ -6,6 +6,7 @@
> detects invalid types being passed.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> 7: 99970934c7 ! 7: 87739268dc hashmap_get takes "const struct hashmap_entry *"
> @@ -6,6 +6,7 @@
> now detects invalid types being passed.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> 8: 8148ed70eb ! 8: 22226b3bb4 hashmap_remove takes "const struct hashmap_entry *"
> @@ -6,6 +6,7 @@
> now detects invalid types being passed.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/blame.c b/blame.c
> --- a/blame.c
> 9: 3756dcc34e ! 9: 0b64282a1f hashmap_put takes "struct hashmap_entry *"
> @@ -6,6 +6,7 @@
> detects invalid types being passed.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> --- a/builtin/fast-export.c
> 10: ff586aa43c ! 10: c2e9201838 introduce container_of macro
> @@ -7,15 +7,16 @@
> and chained hash tables while allowing the compiler to do
> type checking.
>
> - I intend to use this to remove the limitation of "hashmap_entry"
> - being location-dependent and to allow more compile-time type
> - checking.
> + Later patches will use container_of() to remove the limitation
> + of "hashmap_entry" being location-dependent. This will complete
> + the transition to compile-time type checking for the hashmap API.
>
> This macro already exists in our source as "list_entry" in
> list.h and making "list_entry" an alias to "container_of"
> as the Linux kernel has done is a possibility.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> --- a/git-compat-util.h
> 11: 931356f061 ! 11: c709e607b1 hashmap_get_next returns "struct hashmap_entry *"
> @@ -6,6 +6,7 @@
> hashmap_entry being the first field of a struct.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/diff.c b/diff.c
> --- a/diff.c
> @@ -14,10 +15,10 @@
> {
> int i;
> char *got_match = xcalloc(1, pmb_nr);
> -+ struct hashmap_entry *ent = &match->ent;
> ++ struct hashmap_entry *ent;
>
> - for (; match; match = hashmap_get_next(hm, &match->ent)) {
> -+ for (; ent; ent = hashmap_get_next(hm, ent)) {
> ++ for (ent = &match->ent; 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;
> 12: a748b9b0c4 ! 12: 27ba68a7b8 hashmap: use *_entry APIs to wrap container_of
> @@ -11,6 +11,7 @@
> extra parameter to specify the type.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/diff.c b/diff.c
> --- a/diff.c
> @@ -19,9 +20,9 @@
> {
> int i;
> char *got_match = xcalloc(1, pmb_nr);
> -- struct hashmap_entry *ent = &match->ent;
> +- struct hashmap_entry *ent;
>
> -- for (; ent; ent = hashmap_get_next(hm, ent)) {
> +- for (ent = &match->ent; ent; ent = hashmap_get_next(hm, ent)) {
> - match = container_of(ent, struct moved_entry, ent);
> + hashmap_for_each_entry_from(hm, match, struct moved_entry, ent) {
> for (i = 0; i < pmb_nr; i++) {
> @@ -125,7 +126,6 @@
> #define container_of(ptr, type, member) \
> ((type *) ((char *)(ptr) - offsetof(type, member)))
>
> -+
> +/*
> + * helper function for `container_of_or_null' to avoid multiple
> + * evaluation of @ptr
> 13: e8faa5e12e ! 13: 222b1f07f3 hashmap_get{,_from_hash} return "struct hashmap_entry *"
> @@ -9,6 +9,7 @@
> hashmap_entry being the first field in a struct.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> @@ -152,11 +153,13 @@
> }
>
> -void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
> -+struct hashmap_entry *
> -+hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
> - const void *keydata)
> +- const void *keydata)
> ++struct hashmap_entry *hashmap_get(const struct hashmap *map,
> ++ const struct hashmap_entry *key,
> ++ const void *keydata)
> {
> return *find_entry_ptr(map, key, keydata);
> + }
> @@
> /* lookup interned string in pool */
> hashmap_entry_init(&key.ent, memhash(data, len));
> @@ -175,21 +178,27 @@
> * to `hashmap_cmp_fn` to decide whether the entry matches the key.
> */
> -void *hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
> -+struct hashmap_entry *
> -+hashmap_get(const struct hashmap *map, const struct hashmap_entry *key,
> - const void *keydata);
> +- const void *keydata);
> ++struct hashmap_entry *hashmap_get(const struct hashmap *map,
> ++ const struct hashmap_entry *key,
> ++ const void *keydata);
>
> /*
> + * Returns the hashmap entry for the specified hash code and key data,
> @@
> * `entry_or_key` parameter of `hashmap_cmp_fn` points to a hashmap_entry
> * structure that should not be used in the comparison.
> */
> -static inline void *hashmap_get_from_hash(const struct hashmap *map,
> -+static inline struct hashmap_entry *
> -+hashmap_get_from_hash(const struct hashmap *map,
> - unsigned int hash,
> - const void *keydata)
> +- unsigned int hash,
> +- const void *keydata)
> ++static inline struct hashmap_entry *hashmap_get_from_hash(
> ++ const struct hashmap *map,
> ++ unsigned int hash,
> ++ const void *keydata)
> {
> + struct hashmap_entry key;
> + hashmap_entry_init(&key, hash);
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> --- a/merge-recursive.c
> 14: f84bd96b8e ! 14: c789f98c81 hashmap_cmp_fn takes hashmap_entry params
> @@ -6,6 +6,7 @@
> being the first member of a struct.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> 15: 23fbd888d9 ! 15: 57ae5b5142 hashmap: use *_entry APIs for iteration
> @@ -7,6 +7,7 @@
> by compilers lacking __typeof__ support.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> 16: 6908364381 ! 16: f1e35ba094 hashmap: hashmap_{put,remove} return hashmap_entry *
> @@ -6,6 +6,7 @@
> to simplify most callers.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/hashmap.c b/hashmap.c
> --- a/hashmap.c
> @@ -15,18 +16,20 @@
> }
>
> -void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
> -+struct hashmap_entry *
> -+hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
> - const void *keydata)
> +- const void *keydata)
> ++struct hashmap_entry *hashmap_remove(struct hashmap *map,
> ++ const struct hashmap_entry *key,
> ++ const void *keydata)
> {
> struct hashmap_entry *old;
> + struct hashmap_entry **e = find_entry_ptr(map, key, keydata);
> @@
> return old;
> }
>
> -void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry)
> -+struct hashmap_entry *
> -+hashmap_put(struct hashmap *map, struct hashmap_entry *entry)
> ++struct hashmap_entry *hashmap_put(struct hashmap *map,
> ++ struct hashmap_entry *entry)
> {
> struct hashmap_entry *old = hashmap_remove(map, entry, NULL);
> hashmap_add(map, entry);
> @@ -39,8 +42,8 @@
> * Returns the replaced entry, or NULL if not found (i.e. the entry was added).
> */
> -void *hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
> -+struct hashmap_entry *
> -+hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
> ++struct hashmap_entry *hashmap_put(struct hashmap *map,
> ++ struct hashmap_entry *entry);
> +
> +#define hashmap_put_entry(map, keyvar, type, member) \
> + container_of_or_null(hashmap_put(map, &(keyvar)->member), type, member)
> @@ -52,17 +55,17 @@
> * Argument explanation is the same as in `hashmap_get`.
> */
> -void *hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
> -+struct hashmap_entry *
> -+hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
> - const void *keydata);
> -
> +- const void *keydata);
> ++struct hashmap_entry *hashmap_remove(struct hashmap *map,
> ++ const struct hashmap_entry *key,
> ++ const void *keydata);
> ++
> +#define hashmap_remove_entry(map, keyvar, keydata, type, member) \
> + container_of_or_null(hashmap_remove(map, &(keyvar)->member, keydata), \
> + type, member)
> -+
> +
> /*
> * Returns the `bucket` an entry is stored in.
> - * Useful for multithreaded read access.
>
> diff --git a/range-diff.c b/range-diff.c
> --- a/range-diff.c
> 17: 150944128b ! 17: adc7cb5516 hashmap: introduce hashmap_free_entries
> @@ -12,6 +12,7 @@
> the hashmap itself.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/blame.c b/blame.c
> --- a/blame.c
> 18: de212aa6d6 ! 18: 1a68e1645d OFFSETOF_VAR macro to simplify hashmap iterators
> @@ -5,7 +5,7 @@
> 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.
> + pointer arithmetic for compilers without `__typeof__'.
>
> This allows us to simplify usage of hashmap iterator macros
> by not having to specify a type when a pointer of that type
> @@ -16,7 +16,10 @@
> trouble of using container_of/list_entry macros and without
> relying on non-portable `__typeof__'.
>
> + v3: use `__typeof__' to avoid clang warnings
> +
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> @@ -141,11 +144,17 @@
> (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()
> ++ * like offsetof(), but takes a pointer to a a variable of type which
> ++ * contains @member, instead of a specified type.
> ++ * @ptr is subject to multiple evaluation since we can't rely on __typeof__
> ++ * everywhere.
> + */
> ++#if defined(__GNUC__) /* clang sets this, too */
> ++#define OFFSETOF_VAR(ptr, member) offsetof(__typeof__(*ptr), member)
> ++#else /* !__GNUC__ */
> +#define OFFSETOF_VAR(ptr, member) \
> + ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))
> ++#endif /* !__GNUC__ */
> +
> #endif
>
> 19: 212a596edd ! 19: fe02b9f839 hashmap: remove type arg from hashmap_{get,put,remove}_entry
> @@ -10,6 +10,7 @@
> sequential as they are used as: `keyvar->member'
>
> Signed-off-by: Eric Wong <e@80x24.org>
> + Reviewed-by: Derrick Stolee <stolee@gmail.com>
>
> diff --git a/attr.c b/attr.c
> --- a/attr.c
> @@ -147,8 +148,8 @@
> * printf("first: %ld %s\n", e->key, e->value);
> * while ((e = hashmap_get_next_entry(&map, e,
> @@
> - struct hashmap_entry *
> - hashmap_put(struct hashmap *map, struct hashmap_entry *entry);
> + struct hashmap_entry *hashmap_put(struct hashmap *map,
> + struct hashmap_entry *entry);
>
> -#define hashmap_put_entry(map, keyvar, type, member) \
> - container_of_or_null(hashmap_put(map, &(keyvar)->member), type, member)
> @@ -167,8 +168,8 @@
> /*
> * Removes a hashmap entry matching the specified key. If the hashmap contains
> @@
> - hashmap_remove(struct hashmap *map, const struct hashmap_entry *key,
> - const void *keydata);
> + const struct hashmap_entry *key,
> + const void *keydata);
>
> -#define hashmap_remove_entry(map, keyvar, keydata, type, member) \
> - container_of_or_null(hashmap_remove(map, &(keyvar)->member, keydata), \
> -: ---------- > 20: 4ec5200646 hashmap_entry: remove first member requirement from docs
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes
2019-10-08 8:58 ` Johannes Schindelin
@ 2019-10-08 13:56 ` Derrick Stolee
0 siblings, 0 replies; 30+ messages in thread
From: Derrick Stolee @ 2019-10-08 13:56 UTC (permalink / raw)
To: Johannes Schindelin, Eric Wong; +Cc: Junio C Hamano, git, Phillip Wood
On 10/8/2019 4:58 AM, Johannes Schindelin wrote:
> Hi Eric & Junio,
>
> On Sun, 6 Oct 2019, Eric Wong wrote:
>
>> v3 changes:
>> - use __typeof__ to avoid invalid clang warning on uninitialized var
>> - formatting fixes recommended by Stolee
>> - add Reviewed-by for Stolee
>> - add patch 20 to update docs to drop first member requirement
>
> This has quite a bit of fallout, even more than previous rounds, it
> seems. I need at least these fixup???s to fix the build of `pu`:
>
> https://github.com/git-for-windows/git/commit/f74259754971b427a14e6290681e18950824b99d
> https://github.com/git-for-windows/git/commit/124c8bc08e974e76ca7d956dc07eb288e71d639e
> https://github.com/git-for-windows/git/commit/45948433d1b48ff513fbd37f134c0f1491c78192
>
> Junio, feel free to fetch and squash those into the appropriate merge
> commit(s) (I feel that 4432e8b5f36 (Merge branch 'ds/sparse-cone' into
> pu, 2019-10-08) may be the right spot).
I expected that my series would have semantic conflicts with this series [1].
I could re-work my code to take these changes into account, based on an
appropriate merge of ds/include-exclude and ew/hashmap. ew/hashmap is likely
much closer to merging than ds/sparse-cone.
Thanks for doing most of the heavy lifting, Johannes! I'll probably start by
taking your fixups into the appropriate commits of my series.
-Stolee
[1] https://public-inbox.org/git/d16b22c9-c437-0947-fb5a-9421f987a1f1@gmail.com/
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
` (21 preceding siblings ...)
2019-10-08 8:58 ` Johannes Schindelin
@ 2019-10-08 13:59 ` Derrick Stolee
2019-10-09 1:20 ` Junio C Hamano
22 siblings, 1 reply; 30+ messages in thread
From: Derrick Stolee @ 2019-10-08 13:59 UTC (permalink / raw)
To: Eric Wong, Junio C Hamano; +Cc: git, Johannes Schindelin, Phillip Wood
On 10/6/2019 7:30 PM, Eric Wong wrote:
> v3 changes:
> - use __typeof__ to avoid invalid clang warning on uninitialized var
> - formatting fixes recommended by Stolee
> - add Reviewed-by for Stolee
> - add patch 20 to update docs to drop first member requirement
>
> Range-diff against v2:
I scanned the range-diff (and patch 20) and think this series is in good shape.
My Reviewed-by stands.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes
2019-10-08 13:59 ` Derrick Stolee
@ 2019-10-09 1:20 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2019-10-09 1:20 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Eric Wong, git, Johannes Schindelin, Phillip Wood
Derrick Stolee <stolee@gmail.com> writes:
> On 10/6/2019 7:30 PM, Eric Wong wrote:
>> v3 changes:
>> - use __typeof__ to avoid invalid clang warning on uninitialized var
>> - formatting fixes recommended by Stolee
>> - add Reviewed-by for Stolee
>> - add patch 20 to update docs to drop first member requirement
>>
>> Range-diff against v2:
>
> I scanned the range-diff (and patch 20) and think this series is in good shape.
> My Reviewed-by stands.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2019-10-09 1:21 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-06 23:30 [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Eric Wong
2019-10-06 23:30 ` [PATCH v3 01/20] diff: use hashmap_entry_init on moved_entry.ent Eric Wong
2019-10-06 23:30 ` [PATCH v3 02/20] coccicheck: detect hashmap_entry.hash assignment Eric Wong
2019-10-06 23:30 ` [PATCH v3 03/20] packfile: use hashmap_entry in delta_base_cache_entry Eric Wong
2019-10-06 23:30 ` [PATCH v3 04/20] hashmap_entry_init takes "struct hashmap_entry *" Eric Wong
2019-10-06 23:30 ` [PATCH v3 05/20] hashmap_get_next takes "const struct " Eric Wong
2019-10-06 23:30 ` [PATCH v3 06/20] hashmap_add takes "struct " Eric Wong
2019-10-06 23:30 ` [PATCH v3 07/20] hashmap_get takes "const struct " Eric Wong
2019-10-06 23:30 ` [PATCH v3 08/20] hashmap_remove " Eric Wong
2019-10-06 23:30 ` [PATCH v3 09/20] hashmap_put takes "struct " Eric Wong
2019-10-06 23:30 ` [PATCH v3 10/20] introduce container_of macro Eric Wong
2019-10-06 23:30 ` [PATCH v3 11/20] hashmap_get_next returns "struct hashmap_entry *" Eric Wong
2019-10-06 23:30 ` [PATCH v3 12/20] hashmap: use *_entry APIs to wrap container_of Eric Wong
2019-10-06 23:30 ` [PATCH v3 13/20] hashmap_get{,_from_hash} return "struct hashmap_entry *" Eric Wong
2019-10-06 23:30 ` [PATCH v3 14/20] hashmap_cmp_fn takes hashmap_entry params Eric Wong
2019-10-06 23:30 ` [PATCH v3 15/20] hashmap: use *_entry APIs for iteration Eric Wong
2019-10-06 23:30 ` [PATCH v3 16/20] hashmap: hashmap_{put,remove} return hashmap_entry * Eric Wong
2019-10-06 23:30 ` [PATCH v3 17/20] hashmap: introduce hashmap_free_entries Eric Wong
2019-10-06 23:30 ` [PATCH v3 18/20] OFFSETOF_VAR macro to simplify hashmap iterators Eric Wong
2019-10-06 23:30 ` [PATCH v3 19/20] hashmap: remove type arg from hashmap_{get,put,remove}_entry Eric Wong
2019-10-06 23:30 ` [PATCH v3 20/20] hashmap_entry: remove first member requirement from docs Eric Wong
2019-10-07 1:52 ` Junio C Hamano
2019-10-07 8:43 ` [PATCH v3 21/20] convert: drop invalid comment for subprocess_entry Eric Wong
2019-10-08 2:15 ` Junio C Hamano
2019-10-08 2:56 ` Eric Wong
2019-10-07 1:24 ` [PATCH v3 00/20] hashmap bug/safety/ease-of-use fixes Junio C Hamano
2019-10-08 8:58 ` Johannes Schindelin
2019-10-08 13:56 ` Derrick Stolee
2019-10-08 13:59 ` Derrick Stolee
2019-10-09 1:20 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).