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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx/

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux