| Message ID | 20190924010324.22619-1-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 9/23/2019 9:03 PM, 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 :) I like this series a lot. The goal is very noble. As one who recently interacted with the hashmap API for the first time, I would have preferred working with the new API. I do wonder about it conflicting with my sparse-checkout changes, which create a lot of new callers to the API. I suspect that your change will be easier to merge quickly and I will want to rebase on top. Perhaps Junio could chime in with his preferred integration plan? As for review, I have mostly minor comments. There appears to be consistent whitespace issues in hashmap.c and hashmap.h, but maybe I'm reading them incorrectly. Also, consider merging patches 10 & 11 as 11 seems to undo the work in patch 10. Thanks, -Stolee
Hi Eric, On Tue, 24 Sep 2019, 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 :) I won't have time to review this patch series, but I wanted to throw out the idea of storing the offset in `struct hashmap`, i.e. the offset of the `struct hashmap_entry` in the actual entry struct? Example: struct erics_entry { const char *i_want_this_to_be_the_first_field; struct hashmap_entry e; [...] }; [...] struct erics_entry *dummy = NULL; size_t offset = ((char *)dummy->e) - ((char *)dummy); [... store that offset in the hashmap and subtract it from the stored pointers before returning an entry...] IOW rather than assuming that it is the first field, we could allow an offset other than 0. I'm just not sure how to implement this elegantly, but it should be much easier to do this portably than the `typeof()` approach. Ciao, Dscho > > 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 >
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 >
Eric Wong <e@80x24.org> writes: > 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 :) > > Apologies for the delays, been busy with other stuff... Thanks. clang version 6.0.1-10 (which is the version I happen to have installed locally) seems to throw a lot of errors like these: -- -- -- $ make CC=clang config.o CC config.o config.c:1944:50: error: variable 'entry' is uninitialized when used here [-Werror,-Wuninitialized] hashmap_for_each_entry(&cs->config_hash, &iter, entry, ^~~~~ ./hashmap.h:453:20: note: expanded from macro 'hashmap_for_each_entry' OFFSETOF_VAR(var, member)); \ ^~~ ./git-compat-util.h:1346:16: note: expanded from macro 'OFFSETOF_VAR' ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) ^~~ ./hashmap.h:445:61: note: expanded from macro 'hashmap_iter_first_entry_offset' container_of_or_null_offset(hashmap_iter_first(map, iter), offset) ^~~~~~ config.c:1939:34: note: initialize the variable 'entry' to silence this warning struct config_set_element *entry; ^ = NULL 1 error generated. make: *** [Makefile:2365: config.o] Error 1 -- -- -- even though gcc seems to be happy with the source.
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > Hi Eric, > > On Tue, 24 Sep 2019, 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 :) > > I won't have time to review this patch series, but I wanted to throw out > the idea of storing the offset in `struct hashmap`, i.e. the offset of > the `struct hashmap_entry` in the actual entry struct? > > Example: > > struct erics_entry { > const char *i_want_this_to_be_the_first_field; > struct hashmap_entry e; > [...] > }; > > [...] > > struct erics_entry *dummy = NULL; > size_t offset = ((char *)dummy->e) - ((char *)dummy); > [... store that offset in the hashmap and subtract it from the > stored pointers before returning an entry...] > > IOW rather than assuming that it is the first field, we could allow an > offset other than 0. I'm just not sure how to implement this elegantly, > but it should be much easier to do this portably than the `typeof()` > approach. Yeah, that needs to be &dummy->e, but that's the OFFSETOF_VAR macro in patch 18. I think it's quite elegant, but I need to fix a clang warning for it...
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 :) 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