Re: [PATCH v2 00/19] hashmap bug/safety/ease-of-use fixes

[Phillip Wood <phillip.wood123@gmail.com>]

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