Hi Eric
On 24/09/2019 02:03, Eric Wong wrote:
Patches 1-11 are largely unchanged from the original series with the
exception of 2, which is new and posted at:
https://public-inbox.org/git/20190908074953.kux7zz4y7iolqko4@whir/
12-17 take further steps to get us away from hashmap_entry being
the first element, but they're also a bit ugly because __typeof__
isn't portable
18-19 finally brings me to the APIs I want to expose without
relying on __typeof :)
Looking at the overall diff for this series looks a lot nicer with the
extra patches that eliminate most of the explicit calls to
container_of(). Thanks for the improved api and the cocci-check patch as
well.
I've only had time for a quick look through but the patches seem well
ordered and easy to follow. I think there are some line folding issues
where you have wrapped a line when you added the type parameter and then
removed it in a later path without re-flowing the line. Apart from that
the only thing I noticed is that hashmap.h still starts with
* struct long2string {
* struct hashmap_entry ent; // must be the first member!
Is that still the case now that hashmap_{get,put,remove}_entry() use
container_of() and hashmap_init() takes a struct hashmap_entry? That
comment is in a lot of our structure definitions as well.
Best Wishes
Phillip
Apologies for the delays, been busy with other stuff...
Previous discussion starts at:
https://public-inbox.org/git/20190826024332.3403-...@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-v2
for you to fetch changes up to 212a596edd99169b284912b7b70b6280e2fb90e6:
hashmap: remove type arg from hashmap_{get,put,remove}_entry (2019-09-24
00:42:22 +0000)
----------------------------------------------------------------
Eric Wong (19):
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
attr.c | 22 ++---
blame.c | 25 +++---
builtin/describe.c | 21 +++--
builtin/difftool.c | 56 ++++++------
builtin/fast-export.c | 15 ++--
builtin/fetch.c | 30 ++++---
config.c | 24 +++---
contrib/coccinelle/hashmap.cocci | 16 ++++
diff.c | 31 ++++---
diffcore-rename.c | 15 ++--
git-compat-util.h | 33 ++++++++
hashmap.c | 58 ++++++++-----
hashmap.h | 164 +++++++++++++++++++++++++++++-------
merge-recursive.c | 87 ++++++++++---------
name-hash.c | 57 +++++++------
oidmap.c | 20 +++--
oidmap.h | 6 +-
packfile.c | 22 +++--
patch-ids.c | 18 ++--
range-diff.c | 10 +--
ref-filter.c | 31 ++++---
refs.c | 23 +++--
remote.c | 21 +++--
revision.c | 28 +++---
sequencer.c | 44 ++++++----
sub-process.c | 20 +++--
sub-process.h | 4 +-
submodule-config.c | 52 +++++++-----
t/helper/test-hashmap.c | 49 ++++++-----
t/helper/test-lazy-init-name-hash.c | 12 +--
30 files changed, 647 insertions(+), 367 deletions(-)
create mode 100644 contrib/coccinelle/hashmap.cocci
Eric Wong (19):
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
attr.c | 22 ++--
blame.c | 25 +++--
builtin/describe.c | 21 ++--
builtin/difftool.c | 56 ++++++----
builtin/fast-export.c | 15 ++-
builtin/fetch.c | 30 ++---
config.c | 24 ++--
contrib/coccinelle/hashmap.cocci | 16 +++
diff.c | 31 +++---
diffcore-rename.c | 15 ++-
git-compat-util.h | 33 ++++++
hashmap.c | 58 ++++++----
hashmap.h | 164 +++++++++++++++++++++++-----
merge-recursive.c | 87 ++++++++-------
name-hash.c | 57 +++++-----
oidmap.c | 20 ++--
oidmap.h | 6 +-
packfile.c | 22 ++--
patch-ids.c | 18 +--
range-diff.c | 10 +-
ref-filter.c | 31 ++++--
refs.c | 23 +++-
remote.c | 21 ++--
revision.c | 28 +++--
sequencer.c | 44 +++++---
sub-process.c | 20 ++--
sub-process.h | 4 +-
submodule-config.c | 52 +++++----
t/helper/test-hashmap.c | 49 +++++----
t/helper/test-lazy-init-name-hash.c | 12 +-
30 files changed, 647 insertions(+), 367 deletions(-)
create mode 100644 contrib/coccinelle/hashmap.cocci
base-commit: 745f6812895b31c02b29bdfe4ae8e5498f776c26
